Add BOM support for compileIvyDeps, runIvyDeps, and BOM test dependencies #4068
Conversation
Conflicts: docs/modules/ROOT/pages/fundamentals/library-deps.adoc example/fundamentals/library-deps/bom-1-external-bom/build.mill example/fundamentals/library-deps/bom-2-dependency-management/build.mill scalalib/src/mill/scalalib/JavaModule.scala scalalib/src/mill/scalalib/PublishModule.scala scalalib/test/src/mill/scalalib/BomTests.scala
Conflicts: scalalib/src/mill/scalalib/JavaModule.scala scalalib/src/mill/scalalib/PublishModule.scala scalalib/test/src/mill/scalalib/BomTests.scala
This comment was marked as outdated.
This comment was marked as outdated.
|
@alexarchambault not sure if I did the merge right, but seems like it might need more fixes to get a green CI |
| def extraBomIvyDeps: Task[Agg[BomDependency]] = Task.Anon { Agg.empty[BomDependency] } | ||
|
|
||
| /** | ||
| * Dependency management entries that are not meant to be overridden or changed by users. | ||
| * | ||
| * This is mainly used to add dependency management entries of the main module to its test | ||
| * modules. | ||
| */ | ||
| def extraDepManagement: Task[Agg[Dep]] = Task.Anon { Agg.empty[Dep] } |
There was a problem hiding this comment.
Do these really need to be Task.Anons? Not sure about BomDependency, but at least Agg[Dep] is serializable and can be a normal Task right?
There was a problem hiding this comment.
These can be made normal Tasks, yes
| allBomIvyDeps().toSeq.map(_.withConfig(Configuration.compile)) ++ extraBomIvyDeps().toSeq | ||
| val depMgmt = processedDependencyManagement( | ||
| (extraDepManagement().toSeq ++ depManagement().toSeq) | ||
| .map(bindDependency()) | ||
| .map(_.dep) | ||
| ) | ||
|
|
||
| addBoms(bomDeps0, depMgmt, overrideVersions = overrideVersions) |
There was a problem hiding this comment.
Can we refactor processDependency, processCompileDependency, and processRunDependency to share the logic between them via an anonymous task. That will help make it easier to see the core differences between them.
I'm also not clear about the differences here:
processCompileDependencyusescompileDepManagement(), andprocessRunDependencyusesrunDepManagementprocessDependencyandprocessCompileDependencyboth useextraDepManagementbutprocessRunDependencydoes not. Why?- Only
processDependencyseems to useextraBomIvyDeps, whileprocessCompileDependencyandprocessRunDependencydo not. Why?
There was a problem hiding this comment.
I'm trying a different approach to this PR's changes in #4145. Rebasing the PR here on top of it should get rid of these additional methods.
There was a problem hiding this comment.
This should also simplify the way BOMs are handled when taking scopes into account. I've been having issues reconciling Mill's compileIvyDeps / ivyDeps / runIvyDeps (where scopes are known beforehand) with how scopes are handled in BOMs (not known beforehand, dependencies from all scopes are mixed, and BOMs can change the scope if it's undefined).
| * added to them. This should be used when propagating the dependencies transitively | ||
| * to other modules. | ||
| */ | ||
| def processedCompileIvyDeps: Task[Agg[BoundDep]] = Task.Anon { |
There was a problem hiding this comment.
Let's make these normal Tasks rather than Task.Anons?
|
Left some comments. @alexarchambault also please fill out the PR description, that will make it easier to review and maintain since we won't need to reverse engineer what you already have in your head |
|
I think I should close this one. #4154, based on #4145, is a cleaner approach, that I trust more than the developments I pushed here. IIRC, I was having a hard time making sense of the changes here, in particular in the light of #4068 (comment). Better go with #4154 and then #4155, even if this needs to wait a bit for |
With the main task first, the compile-only one second, and the runtime one third This makes it easier to change these methods, add related ones, etc. This is already part of com-lihaoyi#4068. Opening the PR here to make it easier to review the changes here, and those of com-lihaoyi#4068 if the one here gets merged
Checking what the CI says, this embeds #3924 for now