Fix conflicting dependencies between upstream JavaModules (#2735)

Fixes #2732

We introduce a new `localCompileClasspath`, which differs from
`compileClasspath` in that it leaves out the
`transitiveCompileClasspath` from upstream modules and `resolvedIvyDeps`
from third party dependencies.

Also tried to do some cleanup of `JavaModule`, refactoring out a
`transitiveModuleCompileModuleDeps` helper to DRY things up,
re-arranging things so all the `fooModuleDeps` helpers are all together.

Tested via a new unit test
`mill.scalalib.HelloWorldTests.multiModuleClasspaths`, which fails on
master and passes on this PR. This test case asserts the overall "shape"
of the classpaths, which should help rule out an entire class of
mis-configuration w.r.t. how the classpaths are wired up

Author: lihaoyi

build.sc

@@ -935,7 +935,7 @@ object bsp extends MillPublishScalaModule with BuildInfo {
   }
 
   object worker extends MillPublishScalaModule {
-    def compileModuleDeps = Seq(bsp, scalalib, testrunner, runner)
+    def compileModuleDeps = Seq(bsp, scalalib, testrunner, runner) ++ scalalib.compileModuleDeps
     def ivyDeps = Agg(Deps.bsp4j, Deps.sbtTestInterface)
   }
 }

example/basic/4-builtin-commands/build.sc

@@ -129,9 +129,9 @@ Inputs:
 
 > ./mill show foo.compileClasspath
 [
-  ".../foo/compile-resources",
   ".../org/scala-lang/scala-library/2.13.11/scala-library-2.13.11.jar",
   ...
+  ".../foo/compile-resources"
 ]
 
 */
@@ -150,9 +150,9 @@ Inputs:
     ".../foo/src"
   ],
   "foo.compileClasspath": [
-    ".../foo/compile-resources",
     ".../org/scala-lang/scala-library/2.13.11/scala-library-2.13.11.jar",
     ...
+    ".../foo/compile-resources"
   ]
 }
 
@@ -171,9 +171,9 @@ Inputs:
     ".../foo/src"
   ],
   "foo.compileClasspath": [
-    ".../foo/compile-resources",
     ".../org/scala-lang/scala-library/2.13.11/scala-library-2.13.11.jar",
     ...
+    ".../foo/compile-resources"
   ]
 }
 

example/cross/10-static-blog/build.sc

@@ -3,7 +3,7 @@
 // libraries - Commonmark and Scalatags - to deal with Markdown parsing and
 // HTML generation respectively:
 
-import $ivy.`com.lihaoyi::scalatags:0.9.1`, scalatags.Text.all._
+import $ivy.`com.lihaoyi::scalatags:0.12.0`, scalatags.Text.all._
 import $ivy.`com.atlassian.commonmark:commonmark:0.13.1`
 import org.commonmark.parser.Parser
 import org.commonmark.renderer.html.HtmlRenderer

example/misc/3-import-file-ivy/build.sc

@@ -1,5 +1,5 @@
 import mill._, scalalib._
-import $ivy.`com.lihaoyi::scalatags:0.8.2`, scalatags.Text.all._
+import $ivy.`com.lihaoyi::scalatags:0.12.0`, scalatags.Text.all._
 import $file.scalaversion, scalaversion.myScalaVersion
 
 object foo extends RootModule with ScalaModule {

example/misc/4-mill-build-folder/build.sc

@@ -52,14 +52,14 @@ compiling 1 Scala source...
 
 > ./mill run
 Foo.value: <h1>hello</h1>
-scalatagsVersion: 0.8.2
+scalatagsVersion: 0.12.0
 
 > ./mill show assembly
 ".../out/assembly.dest/out.jar"
 
 > ./out/assembly.dest/out.jar # mac/linux
 Foo.value: <h1>hello</h1>
-scalatagsVersion: 0.8.2
+scalatagsVersion: 0.12.0
 
 */
 

example/misc/4-mill-build-folder/mill-build/build.sc

@@ -1,7 +1,7 @@
 import mill._, scalalib._
 
 object millbuild extends MillBuildRootModule{
-  val scalatagsVersion = "0.8.2"
+  val scalatagsVersion = "0.12.0"
   def ivyDeps = Agg(ivy"com.lihaoyi::scalatags:$scalatagsVersion")
 
   def generatedSources = T {

example/thirdparty/jimfs/build.sc

@@ -1,6 +1,16 @@
 import mill._, scalalib._, publish._
 
-object jimfs extends PublishModule with MavenModule{
+def sharedCompileIvyDeps = T{
+  Agg(
+    ivy"com.google.auto.service:auto-service:1.0.1",
+    ivy"com.google.code.findbugs:jsr305:3.0.2",
+    ivy"org.checkerframework:checker-compat-qual:2.5.5",
+    ivy"com.ibm.icu:icu4j:73.1",
+  )
+}
+
+
+object jimfs extends PublishModule with MavenModule {
   def publishVersion = "1.3.3.7"
 
   def pomSettings = PomSettings(
@@ -12,21 +22,14 @@ object jimfs extends PublishModule with MavenModule{
     developers = Nil
   )
 
-  def ivyDeps = Agg(
+  def ivyDeps = sharedCompileIvyDeps() ++ Agg(
     ivy"com.google.guava:guava:31.1-android",
   )
 
-  def compileIvyDeps = Agg(
-    ivy"com.google.auto.service:auto-service:1.0.1",
-    ivy"com.google.code.findbugs:jsr305:3.0.2",
-    ivy"org.checkerframework:checker-compat-qual:2.5.5",
-    ivy"com.ibm.icu:icu4j:73.1",
-  )
-
   def javacOptions = Seq("-processor", "com.google.auto.service.processor.AutoServiceProcessor")
 
-  object test extends MavenModuleTests{
-    def ivyDeps = Agg(
+  object test extends MavenModuleTests {
+    def ivyDeps = sharedCompileIvyDeps() ++ Agg(
       ivy"junit:junit:4.13.2",
       ivy"com.google.guava:guava-testlib:31.1-android",
       ivy"com.google.truth:truth:1.1.3",

scalalib/src/mill/scalalib/JavaModule.scala

@@ -128,12 +128,34 @@ trait JavaModule
   /** The compile-only direct dependencies of this module. */
   def compileModuleDeps: Seq[JavaModule] = Seq.empty
 
+  /** The direct and indirect dependencies of this module */
+  def recursiveModuleDeps: Seq[JavaModule] = {
+    moduleDeps.flatMap(_.transitiveModuleDeps).distinct
+  }
+
+  /**
+   * Like `recursiveModuleDeps` but also include the module itself,
+   * basically the modules whose classpath are needed at runtime
+   */
+  def transitiveModuleDeps: Seq[JavaModule] = Seq(this) ++ recursiveModuleDeps
+
+  /**
+   * All direct and indirect module dependencies of this module, including
+   * compile-only dependencies: basically the modules whose classpath are needed
+   * at compile-time.
+   *
+   * Note that `compileModuleDeps` are defined to be non-transitive, so we only
+   * look at the direct `compileModuleDeps` when assembling this list
+   */
+  def transitiveModuleCompileModuleDeps: Seq[JavaModule] = {
+    (moduleDeps ++ compileModuleDeps).flatMap(_.transitiveModuleDeps).distinct
+  }
+
   /** The compile-only transitive ivy dependencies of this module and all it's upstream compile-only modules. */
   def transitiveCompileIvyDeps: T[Agg[BoundDep]] = T {
     // We never include compile-only dependencies transitively, but we must include normal transitive dependencies!
-    compileIvyDeps().map(bindDependency()) ++ T
-      .traverse(compileModuleDeps)(_.transitiveIvyDeps)()
-      .flatten
+    compileIvyDeps().map(bindDependency()) ++
+      T.traverse(compileModuleDeps)(_.transitiveIvyDeps)().flatten
   }
 
   /**
@@ -158,16 +180,6 @@ trait JavaModule
     T.log.outputStream.println(asString)
   }
 
-  /** The direct and indirect dependencies of this module */
-  def recursiveModuleDeps: Seq[JavaModule] = {
-    moduleDeps.flatMap(_.transitiveModuleDeps).distinct
-  }
-
-  /** Like `recursiveModuleDeps` but also include the module itself */
-  def transitiveModuleDeps: Seq[JavaModule] = {
-    Seq(this) ++ recursiveModuleDeps
-  }
-
   /**
    * Additional jars, classfiles or resources to add to the classpath directly
    * from disk rather than being downloaded from Maven Central or other package
@@ -180,28 +192,22 @@ trait JavaModule
    * This is calculated from [[ivyDeps]], [[mandatoryIvyDeps]] and recursively from [[moduleDeps]].
    */
   def transitiveIvyDeps: T[Agg[BoundDep]] = T {
-    (ivyDeps() ++ mandatoryIvyDeps()).map(bindDependency()) ++ T.traverse(moduleDeps)(
-      _.transitiveIvyDeps
-    )().flatten
+    (ivyDeps() ++ mandatoryIvyDeps()).map(bindDependency()) ++
+      T.traverse(moduleDeps)(_.transitiveIvyDeps)().flatten
   }
 
   /**
    * The upstream compilation output of all this module's upstream modules
    */
   def upstreamCompileOutput: T[Seq[CompilationResult]] = T {
-    T.traverse((recursiveModuleDeps ++ compileModuleDeps.flatMap(
-      _.transitiveModuleDeps
-    )).distinct)(_.compile)
+    T.traverse(transitiveModuleCompileModuleDeps)(_.compile)
   }
 
   /**
    * The transitive version of `localClasspath`
    */
   def transitiveLocalClasspath: T[Agg[PathRef]] = T {
-    T.traverse(
-      (moduleDeps ++ compileModuleDeps).flatMap(_.transitiveModuleDeps).distinct
-    )(m => m.localClasspath)()
-      .flatten
+    T.traverse(transitiveModuleCompileModuleDeps)(_.localClasspath)().flatten
   }
 
   /**
@@ -210,20 +216,16 @@ trait JavaModule
   // Keep in sync with [[transitiveLocalClasspath]]
   @internal
   def bspTransitiveLocalClasspath: T[Agg[UnresolvedPath]] = T {
-    T.traverse(
-      (moduleDeps ++ compileModuleDeps).flatMap(_.transitiveModuleDeps).distinct
-    )(m => m.bspLocalClasspath)()
-      .flatten
+    T.traverse(transitiveModuleCompileModuleDeps)(_.bspLocalClasspath)().flatten
   }
 
   /**
    * The transitive version of `compileClasspath`
    */
   def transitiveCompileClasspath: T[Agg[PathRef]] = T {
-    T.traverse(
-      (moduleDeps ++ compileModuleDeps).flatMap(_.transitiveModuleDeps).distinct
-    )(m => T.task { m.compileClasspath() ++ Agg(m.compile().classes) })()
-      .flatten
+    T.traverse(transitiveModuleCompileModuleDeps)(m =>
+      T.task { m.localCompileClasspath() ++ Agg(m.compile().classes) }
+    )().flatten
   }
 
   /**
@@ -232,9 +234,7 @@ trait JavaModule
   // Keep in sync with [[transitiveCompileClasspath]]
   @internal
   def bspTransitiveCompileClasspath: T[Agg[UnresolvedPath]] = T {
-    T.traverse(
-      (moduleDeps ++ compileModuleDeps).flatMap(_.transitiveModuleDeps).distinct
-    )(m =>
+    T.traverse(transitiveModuleCompileModuleDeps)(m =>
       T.task {
         m.bspCompileClasspath() ++ Agg(m.bspCompileClassesPath())
       }
@@ -355,11 +355,11 @@ trait JavaModule
     }
 
   /**
-   * The output classfiles/resources from this module, excluding upstream
-   * modules and third-party dependencies
+   * The *output* classfiles/resources from this module, used for execution,
+   * excluding upstream modules and third-party dependencies
    */
   def localClasspath: T[Seq[PathRef]] = T {
-    compileResources() ++ resources() ++ Agg(compile().classes)
+    localCompileClasspath().toSeq ++ resources() ++ Agg(compile().classes)
   }
 
   /**
@@ -368,9 +368,8 @@ trait JavaModule
    */
   @internal
   def bspLocalClasspath: T[Agg[UnresolvedPath]] = T {
-    (compileResources() ++ resources()).map(p => UnresolvedPath.ResolvedPath(p.path)) ++ Agg(
-      bspCompileClassesPath()
-    )
+    (compileResources() ++ resources()).map(p => UnresolvedPath.ResolvedPath(p.path)) ++
+      Agg(bspCompileClassesPath())
   }
 
   /**
@@ -379,53 +378,51 @@ trait JavaModule
    */
   // Keep in sync with [[bspCompileClasspath]]
   def compileClasspath: T[Agg[PathRef]] = T {
-    transitiveCompileClasspath() ++
-      compileResources() ++
-      unmanagedClasspath() ++
-      resolvedIvyDeps()
+    resolvedIvyDeps() ++ transitiveCompileClasspath() ++ localCompileClasspath()
+  }
+
+  /**
+   * The *input* classfiles/resources from this module, used during compilation,
+   * excluding upstream modules and third-party dependencies
+   */
+  def localCompileClasspath: T[Agg[PathRef]] = T {
+    compileResources() ++ unmanagedClasspath()
   }
 
   /** Same as [[compileClasspath]], but does not trigger compilation targets, if possible. */
   // Keep in sync with [[compileClasspath]]
   @internal
   def bspCompileClasspath: T[Agg[UnresolvedPath]] = T {
     bspTransitiveCompileClasspath() ++
-      (compileResources() ++ unmanagedClasspath() ++ resolvedIvyDeps())
+      (localCompileClasspath() ++ resolvedIvyDeps())
         .map(p => UnresolvedPath.ResolvedPath(p.path))
   }
 
   /**
    * Resolved dependencies based on [[transitiveIvyDeps]] and [[transitiveCompileIvyDeps]].
    */
   def resolvedIvyDeps: T[Agg[PathRef]] = T {
-    resolveDeps(T.task {
-      transitiveCompileIvyDeps() ++ transitiveIvyDeps()
-    })()
+    resolveDeps(T.task { transitiveCompileIvyDeps() ++ transitiveIvyDeps() })()
   }
 
   /**
    * All upstream classfiles and resources necessary to build and executable
    * assembly, but without this module's contribution
    */
   def upstreamAssemblyClasspath: T[Agg[PathRef]] = T {
-    transitiveLocalClasspath() ++
-      unmanagedClasspath() ++
-      resolvedRunIvyDeps()
+    resolvedRunIvyDeps() ++ transitiveLocalClasspath()
   }
 
   def resolvedRunIvyDeps: T[Agg[PathRef]] = T {
-    resolveDeps(T.task {
-      runIvyDeps().map(bindDependency()) ++ transitiveIvyDeps()
-    })()
+    resolveDeps(T.task { runIvyDeps().map(bindDependency()) ++ transitiveIvyDeps() })()
   }
 
   /**
    * All classfiles and resources from upstream modules and dependencies
    * necessary to run this module's code after compilation
    */
   def runClasspath: T[Seq[PathRef]] = T {
-    localClasspath() ++
-      upstreamAssemblyClasspath()
+    resolvedRunIvyDeps().toSeq ++ transitiveLocalClasspath() ++ localClasspath()
   }
 
   /**
@@ -470,10 +467,7 @@ trait JavaModule
    * without those from upstream modules and dependencies
    */
   def jar: T[PathRef] = T {
-    Jvm.createJar(
-      localClasspath().map(_.path).filter(os.exists),
-      manifest()
-    )
+    Jvm.createJar(localClasspath().map(_.path).filter(os.exists), manifest())
   }
 
   /**