Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude trees from ammonite dep. #941

Merged
merged 1 commit into from
Nov 3, 2020
Merged

Conversation

ckipp01
Copy link
Contributor

@ckipp01 ckipp01 commented Jul 28, 2020

The reason for doing this is to ensure that the version of
trees that mill wants is indeeed used instead of the newer
version that is being pulled in by ammonite-interp. Before
this was using forceVersion(), which was added 438ebc...6b492a, but by doing this the forced
version doesn't end up in the POM meaning that when someone
bootstraps/uses mill via coursier, they are getting the newer
trees, which is causing issues with how the main class is detected.

You can see we are getting 4.3.20 here instead of the forced 4.3.7:

❯ cs resolve mill | grep trees
org.scalameta:trees_2.13:4.3.20:default

This is coming from ammonite-interp:

❯ cs resolve mill --what-depends-on org.scalameta:trees_2.13
  Result:
└─ org.scalameta:trees_2.13:4.3.20
   ├─ com.lihaoyi:ammonite-interp_2.13.2:2.2.0
   │  ├─ com.lihaoyi:ammonite-repl_2.13.2:2.2.0
   │  │  └─ com.lihaoyi:ammonite_2.13.2:2.2.0
   │  │     └─ com.lihaoyi:mill-main-core_2.13:0.8.0
   │  │        └─ com.lihaoyi:mill-main_2.13:0.8.0
   │  │           └─ com.lihaoyi:mill-scalalib_2.13:0.8.0
   │  │              ├─ com.lihaoyi:mill-dev_2.13:0.8.0
   │  │              ├─ com.lihaoyi:mill-scalajslib_2.13:0.8.0
   │  │              │  └─ com.lihaoyi:mill-dev_2.13:0.8.0
   │  │              └─ com.lihaoyi:mill-scalanativelib_2.13:0.8.0
   │  │                 └─ com.lihaoyi:mill-dev_2.13:0.8.0
   │  └─ com.lihaoyi:ammonite_2.13.2:2.2.0
   │     └─ com.lihaoyi:mill-main-core_2.13:0.8.0
   │        └─ com.lihaoyi:mill-main_2.13:0.8.0
   │           └─ com.lihaoyi:mill-scalalib_2.13:0.8.0
   │              ├─ com.lihaoyi:mill-dev_2.13:0.8.0
   │              ├─ com.lihaoyi:mill-scalajslib_2.13:0.8.0
   │              │  └─ com.lihaoyi:mill-dev_2.13:0.8.0
   │              └─ com.lihaoyi:mill-scalanativelib_2.13:0.8.0
   │                 └─ com.lihaoyi:mill-dev_2.13:0.8.0
   └─ com.lihaoyi:mill-main-core_2.13:0.8.0 org.scalameta:trees_2.13:4.3.7 -> 4.3.20
      └─ com.lihaoyi:mill-main_2.13:0.8.0
         └─ com.lihaoyi:mill-scalalib_2.13:0.8.0
            ├─ com.lihaoyi:mill-dev_2.13:0.8.0
            ├─ com.lihaoyi:mill-scalajslib_2.13:0.8.0
            │  └─ com.lihaoyi:mill-dev_2.13:0.8.0
            └─ com.lihaoyi:mill-scalanativelib_2.13:0.8.0
               └─ com.lihaoyi:mill-dev_2.13:0.8.0

After this change you can see that now the desired 4.3.7 is here:

❯ cs resolve mill:0.8.0-2-b4e258 --what-depends-on org.scalameta:trees_2.13
  Result:
└─ org.scalameta:trees_2.13:4.3.7
   └─ com.lihaoyi:mill-main-core_2.13:0.8.0-2-b4e258
      └─ com.lihaoyi:mill-main_2.13:0.8.0-2-b4e258
         └─ com.lihaoyi:mill-scalalib_2.13:0.8.0-2-b4e258
            ├─ com.lihaoyi:mill-dev_2.13:0.8.0-2-b4e258
            ├─ com.lihaoyi:mill-scalajslib_2.13:0.8.0-2-b4e258
            │  └─ com.lihaoyi:mill-dev_2.13:0.8.0-2-b4e258
            └─ com.lihaoyi:mill-scalanativelib_2.13:0.8.0-2-b4e258
               └─ com.lihaoyi:mill-dev_2.13:0.8.0-2-b4e258

You can find more info about all of this in detail here: coursier/apps#56 thanks to @alexarchambault and his investigative skills.

@lihaoyi
Copy link
Member

lihaoyi commented Jul 28, 2020

but by doing this the forced
version doesn't end up in the POM meaning that when someone
bootstraps/uses mill via coursier, they are getting the newer
trees, which is causing issues with how the main class is detected.

Does anyone know if POM files have a way of forcing dependencies? IIRC there was a <dependencyManagement> section that could be used to pin versions; if so, we should fix Mill to generate a version-pinning pom rather than doing an exclusion

@ckipp01
Copy link
Contributor Author

ckipp01 commented Jul 28, 2020

Does anyone know if POM files have a way of forcing dependencies? IIRC there was a section that could be used to pin versions; if so, we should fix Mill to generate a version-pinning pom rather than doing an exclusion

But wouldn't the most common way to do this using <dependencyManagement> just be to also use an exclusion?

<dependencyManagement>
	<dependency>
	  <groupId>com.lihaoyi</groupId>
	  <artifactId>ammonite-interp_2.13</artifactId>
	  <version></version>
	  <exclusions>
	    <exclusion>
	      <artifactId>org.scalameta</artifactId>
	      <groupId>trees_2.13</groupId>
	    </exclusion>
	  </exclusions>
	<dependency>
</dependencyManagement>

Or something similar to that?

@alexarchambault
Copy link
Collaborator

Does anyone know if POM files have a way of forcing dependencies?

There aren't AFAIK. The dependency management of transitive dependencies cannot be used to force versions of dependencies they pulled. (I just tested that in this project for confirmation).

If project A transitively depends on foo:2.2, but puts foo:1.3 in its dependency management, foo:1.3 will be pulled when building A from Maven. But if A gets published, and project B depends on A, B will pull foo:2.2 via A.

I had thought about having coursier depart from how Maven does, and use dependency management as a way to force versions too. But for now, it just does the same as Maven.

@ckipp01
Copy link
Contributor Author

ckipp01 commented Aug 9, 2020

Does anyone know if POM files have a way of forcing dependencies? IIRC there was a section that could be used to pin versions; if so, we should fix Mill to generate a version-pinning pom rather than doing an exclusion

Since this doesn't seem to be possible. Is the exclusion alright?

@lefou
Copy link
Member

lefou commented Nov 2, 2020

Ok, after reading this PR again I understand the context a bit better and think we want this PR merged. Essentially, it tries to work around some incompatible transitive dependencies by excluding and explicitly listing them.

Chris (@ckipp01), it would be very nice to have a code comment (explaining that fact) as part of the patch, to make future maintenance more easy.

The reason for doing this is to ensure that the version of
trees that mill wants is indeed used instead of the newer
version that is being pulled in by ammonite-interp. Before
this was using `forceVersion()`, but by doing this the forced
version doesn't end up in the POM meaning that when someone
bootstraps/uses mill via coursier, they are getting the newer
trees, which is causing issues with how the main class is detected.

You can see more details on this coursier/apps#56
and a huge props to @alexarchambault for finding this.
@ckipp01
Copy link
Contributor Author

ckipp01 commented Nov 3, 2020

Ok, after reading this PR again I understand the context a bit better and think we want this PR merged. Essentially, it tries to work around some incompatible transitive dependencies by excluding and explicitly listing them.

Chris (@ckipp01), it would be very nice to have a code comment (explaining that fact) as part of the patch, to make future maintenance more easy.

Sure, I've gone ahead and added a short description in to explain why the exclude instead of the forceVersion.

@lefou lefou merged commit 0dcbd73 into com-lihaoyi:master Nov 3, 2020
@lefou lefou added this to the after 0.8.0 milestone Nov 3, 2020
@lefou
Copy link
Member

lefou commented Nov 3, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants