Overhaul transitive module handling in dependency resolution #4145
Conversation
This changes the way transitive dependencies, but also transitive BOM dependencies and dependency management, are passed to coursier. Before this commit, Mill was walking its own module graph, and dependencies / BOM dependencies / dep management were manually aggregated beforehand. During dependency resolution, coursier also does such aggregations. Having both Mill and coursier handle that concern could lead to discrepancies. So this commit passes non-processed dependencies / BOM dependencies / dep management as-is to coursier, and lets coursier handle / compose those.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
These ought to be replaced by having those plugins use compile-time dependencies for most of their own dependencies
By marking all worker dependencies as provided (aka compile-time only)
This comment was marked as resolved.
This comment was marked as resolved.
This PR makes Mill stop relying on the `resolveFilter` parameter of `CoursierSupport#resolveDependencies`. This parameter manually filters the artifacts provided by coursier, but not the ones originating from "local dependencies" (Mill's hack to use freshly built workers and plugins during tests), where it seems such a filtering was done in the build (by customizing `testDepPaths`). "Local dependencies" cannot be kept as-is in #4145 (where such dependencies aren't necessarily root dependencies anymore, making it harder to handle them in a different fashion). Hence the need to stop using `resolveFilter`. Rather than filtering artifacts after dependency resolution, this PR marks unwanted dependencies of workers as compile-time only. That way, they're not pulled by dependency resolution, and we don't need to manually filter artifacts.
|
I think this looks good to me. There are some classpath re-orderings, but we don't generally promise stable classpath orderings anyway. The |
| @@ -84,6 +84,8 @@ trait CoursierModule extends mill.Module { | |||
| */ | |||
| def mapDependencies: Task[Dependency => Dependency] = Task.Anon { (d: Dependency) => d } | |||
|
|
|||
| def internalRepositories: Task[Seq[Repository]] = Task.Anon(Nil) | |||
There was a problem hiding this comment.
Docs is missing. What is this? How it differentiates to repositoriesTask?
There was a problem hiding this comment.
Adding some. The internal in-memory repo with the coursier projects of Mill's modules is meant to go there.
There was a problem hiding this comment.
Can you explain the role of the internal repositories? It looks like some essential implementation detail, but is exposed as task. Maybe, it's visibility can be reduced or it can be moved into some worker.
You introduced a new def coursierDependency which seems to represent the module itself. Is this in the spirit of PublishModule.publishSelfDependency? It seems to ignore any user settings regarding the artifact name and organization. Is it only used internally? What impact has it, when it's different from what publishSelfDependency returns?
| * The `coursier.Dependency` to use to refer to this module | ||
| */ | ||
| def processDependency( | ||
| overrideVersions: Boolean = false | ||
| ): Task[coursier.core.Dependency => coursier.core.Dependency] = Task.Anon { | ||
| val bomDeps0 = allBomDeps().toSeq.map(_.withConfig(Configuration.compile)) | ||
| val depMgmt = processedDependencyManagement( | ||
| depManagement().toSeq.map(bindDependency()).map(_.dep) | ||
| def coursierDependency: cs.Dependency = |
There was a problem hiding this comment.
This is ignoring all user customizations to artifactName and artifactId.
There was a problem hiding this comment.
That's on purpose for now, yes
It has some scaladoc in |
It's only used internally, yes. I thought about making it use the same coordinates as for publishing, but I fear it might introduce ambiguity, if users mistakenly give the same coordinates to two modules. Also, all There shouldn't be any impact… These internal coordinates leak in the dependency trees, but some post-processing (added in this PR) hides those (it actually only keeps the internal name, built out of the module segments, which should look familiar enough to users). |
There was a problem hiding this comment.
@alexarchambault Thanks for the insights. I think, we should mark all these internal methods with @internal and reduce their visibility as much as possible so that we can move/remove them later. Ideally, we can group and hide them in some inner coursier object in the future.
Besides, this PR looks good to me.
| @@ -243,8 +243,7 @@ out/mill-server | |||
|
|
|||
| > cat out/mill-build/methodCodeHashSignatures.dest/current/spanningInvalidationTree.json | |||
| { | |||
| "call scala.runtime.BoxesRunTime.boxToInteger(int)java.lang.Integer": {}, | |||
| "call scala.Predef$#println(java.lang.Object)void": { | |||
| "call scala.runtime.BoxesRunTime.boxToInteger(int)java.lang.Integer": { | |||
There was a problem hiding this comment.
Just wanted to mention that I'm not familiar enough with the hash signature stuff to really understand why this change is needed. Oddly enough, that needs to be reverted in #4155.
| }, | ||
| { | ||
| "name": "org.hamcrest:hamcrest-core", | ||
| "version": "1.3" |
There was a problem hiding this comment.
The changes in this file are a good thing: rather than including only dependencies directly added in Mill modules (via transitiveIvyDeps and all), this adds the whole dependency graph (including transitive dependencies pulled externally). IIUC, this is what is expected from that BSP request.
| @@ -11,7 +11,7 @@ | |||
| <orderEntry type="library" name="scala-js-SDK-3.3.1" level="project"/> | |||
| <orderEntry type="library" name="portable-scala-reflect_sjs1_2.13-1.1.3.jar" level="project"/> | |||
| <orderEntry type="library" name="scala-library-2.13.13.jar" level="project"/> | |||
| <orderEntry type="library" name="scala3-library_sjs1_3-3.3.1.jar" level="project"/> | |||
| <orderEntry type="library" name="scala3-library_sjs1_3-3.3.3.jar" level="project"/> | |||
There was a problem hiding this comment.
Didn't investigate why the version is bumped here. That doesn't look like a major issue though…
| |│ └─ com.lihaoyi:upickle-core_2.13:1.4.0 | ||
| |│ └─ com.lihaoyi:geny_2.13:0.6.10 | ||
| |└─ org.scala-lang:scala-library:2.13.10 | ||
| """├─ StandaloneScala213 |
There was a problem hiding this comment.
We know see the moduleDep in this tree (and some of the next ones too), which is a good thing IMO
| */ | ||
| def webJarDeps = Task { | ||
| transitiveIvyDeps().filter(_.dep.module.organization.value == "org.webjars") | ||
| ivyDeps() |
There was a problem hiding this comment.
This slightly changes the semantics of the previous code. Before, ivyDeps of the moduleDeps (and their own moduleDeps, etc.) were taken into account. Now, only the ivyDeps of the current module are.
Alternatively, we could have a look at the whole dependency graph (including external transitive dependencies). But only looking at direct dependencies of the Mill modules looks odd to me (what if the Mill project is split or is a merge of two Mill projects - that changes the value of this task, while it shouldn't IMO).
|
@alexarchambault feel free to merge this whenever you're ready |
| ProblemFilter.exclude[ReversedMissingMethodProblem]("mill.scalalib.JavaModule.mill$scalalib$JavaModule$$super$internalRepositories"), | ||
| ProblemFilter.exclude[ReversedMissingMethodProblem]("mill.scalanativelib.ScalaNativeModule.mill$scalanativelib$ScalaNativeModule$$super$coursierProject"), | ||
|
|
There was a problem hiding this comment.
@alexarchambault these turn out not to be safe, we probably need to fix the code to not add calls to super since they are not binary compatible
CC @sake92
There was a problem hiding this comment.
I'm not sure I said it before, but I suggest to generate the access to super.<overridden> from our macros as soon as we can break bin-compat, so that we better prepare the binary code for ad-hoc overriding of tasks. The super-calls can be left unused, so they should not affect functionality, but we at least we trigger the super-accessors to be present in the binary code.
There was a problem hiding this comment.
Trying to address that in #4380, although the workarounds aren't fully satisfying IMO (they assume users don't add overrides between the trait defining the method and the one overriding it)
There was a problem hiding this comment.
Other such changes adding calls to super have been introduced in the past, apparently:
Lines 523 to 580 in 9815c35
This PR changes the way transitive dependencies, but also transitive BOM dependencies and dependency management, are passed to coursier.
Before this commit, Mill was walking its own module graph, and dependencies / BOM dependencies / dep management were manually aggregated beforehand. During dependency resolution, coursier also does such aggregations for external dependencies.
Having both Mill and coursier handle that concern could lead to discrepancies or duplicate work (in #4068 in particular). So this commit passes non-processed dependencies / BOM dependencies / dep management as-is to coursier, and lets coursier handle / compose those.
With this PR, dependencies / BOM dependencies / dep management are passed around solely via
JavaModule#coursierProject/JavaModule#transitiveCoursierProjects. This PR deprecatesJavaModule#{transitiveIvyDeps,transitiveCompileIvyDeps,transitiveRunIvyDeps,processDependency,processedIvyDeps}(some only recently introduced in #3924).