Make runtime deps transitive (#3774)

Fixes #3761

* Introduce `runModuleDeps`, the runtime version of `moduleDeps` and
`compileModuleDeps`

* We introduce `transitiveRunIvyDeps`, the runtime version of
`transitiveIvyDeps`, and use it in most places as a replacement for
`runIvyDeps`

* I also flipped the ordering of
`transitiveModuleDeps`/`transitiveRunModuleDeps` to put `this` on the
right/last, to follow the rest of the module resolution logic where
"upstream" things come on the left/first in the classpath ordering

There are some behavioral changes, e.g. the A module's direct
`compileModuleDeps` no longer end up on your `runClasspath`, as shown by
the change in the test case
`mill.scalalib.ScalaMultiModuleClasspathsTests.modCompile`. I think the
new behavior is more correct than the old one?

One (benign?) consequence of this change is that the contents of the
various `*run*` tasks now have considerable overlap with the non-`run`
tasks, e.g. `transitiveRunIvyDeps` overlaps with `transitiveIvyDeps`.

The way transitive runtime dependencies are now managed, both
"`run{Module,Ivy}Dep` of `{module,ivy}Dep`" and "`{module,ivy}Dep` of
`run{Module,Ivy}Dep`" are both aggregated into the run classpath. I'm
not sure if this matches what Maven does (it does according to chatgpt!)
but it seems reasonable to me

Added some additional unit tests to cover the new transitive behavior.
The whole dependency-wiring stuff is pretty gnarly but hopefully the
existing test suite will stop us from breaking too much (especially
`mill.scalalib.ScalaMultiModuleClasspathsTests` which is pretty rigorous
and we add to).

This is a binary-compatible but semantically incompatible change, would
be good to get it into 0.12.0

Best reviewed with `Hide Whitespace` enabled

Author: lihaoyi

integration/feature/full-run-logs/src/FullRunLogsTests.scala

@@ -27,26 +27,26 @@ object FullRunLogsTests extends UtestIntegrationTestSuite {
 
       val res = eval(("--ticker", "true", "run", "--text", "hello"))
       res.isSuccess ==> true
-      assert(res.out == "[46] <h1>hello</h1>")
+      assert("\\[\\d+\\] <h1>hello</h1>".r.matches(res.out))
 
       val expectedErrorRegex =
         s"""==================================================== run --text hello ================================================
            |======================================================================================================================
-           |[build.mill-56/60] compile
-           |[build.mill-56] [info] compiling 1 Scala source to ${tester.workspacePath}/out/mill-build/compile.dest/classes ...
-           |[build.mill-56] [info] done compiling
-           |[40/46] compile
-           |[40] [info] compiling 1 Java source to ${tester.workspacePath}/out/compile.dest/classes ...
-           |[40] [info] done compiling
-           |[46/46] run
-           |[46/46] ============================================ run --text hello ============================================<seconds-digits>s
+           |[build.mill-<digits>/<digits>] compile
+           |[build.mill-<digits>] [info] compiling 1 Scala source to ${tester.workspacePath}/out/mill-build/compile.dest/classes ...
+           |[build.mill-<digits>] [info] done compiling
+           |[<digits>/<digits>] compile
+           |[<digits>] [info] compiling 1 Java source to ${tester.workspacePath}/out/compile.dest/classes ...
+           |[<digits>] [info] done compiling
+           |[<digits>/<digits>] run
+           |[<digits>/<digits>] ============================================ run --text hello ============================================<digits>s
            |======================================================================================================================"""
           .stripMargin
           .replaceAll("(\r\n)|\r", "\n")
           .replace('\\', '/')
-          .split("<seconds-digits>")
+          .split("<digits>")
           .map(java.util.regex.Pattern.quote)
-          .mkString("=? [\\d]+")
+          .mkString("=? ?[\\d]+")
 
       assert(expectedErrorRegex.r.matches(res.err.replace('\\', '/').replaceAll("(\r\n)|\r", "\n")))
     }

scalalib/src/mill/scalalib/JavaModule.scala

@@ -190,6 +190,16 @@ trait JavaModule
     moduleDeps
   }
 
+  /**
+   * Same as [[moduleDeps]] but checked to not contain cycles.
+   * Prefer this over using [[moduleDeps]] directly.
+   */
+  final def runModuleDepsChecked: Seq[JavaModule] = {
+    // trigger initialization to check for cycles
+    recRunModuleDeps
+    runModuleDeps
+  }
+
   /** Should only be called from [[moduleDepsChecked]] */
   private lazy val recModuleDeps: Seq[JavaModule] =
     ModuleUtils.recursive[JavaModule](
@@ -198,9 +208,26 @@ trait JavaModule
       _.moduleDeps
     )
 
-  /** The compile-only direct dependencies of this module. */
+  /** Should only be called from [[moduleDepsChecked]] */
+  private lazy val recRunModuleDeps: Seq[JavaModule] =
+    ModuleUtils.recursive[JavaModule](
+      (millModuleSegments ++ Seq(Segment.Label("moduleDeps"))).render,
+      this,
+      m => m.runModuleDeps ++ m.moduleDeps
+    )
+
+  /**
+   *  The compile-only direct dependencies of this module. These are *not*
+   *  transitive, and only take effect in the module that they are declared in.
+   */
   def compileModuleDeps: Seq[JavaModule] = Seq.empty
 
+  /**
+   * The runtime-only direct dependencies of this module. These *are* transitive,
+   * and so get propagated to downstream modules automatically
+   */
+  def runModuleDeps: Seq[JavaModule] = Seq.empty
+
   /** Same as [[compileModuleDeps]] but checked to not contain cycles. */
   final def compileModuleDepsChecked: Seq[JavaModule] = {
     // trigger initialization to check for cycles
@@ -217,16 +244,22 @@ trait JavaModule
     )
 
   /** The direct and indirect dependencies of this module */
-  def recursiveModuleDeps: Seq[JavaModule] = {
-//    moduleDeps.flatMap(_.transitiveModuleDeps).distinct
-    recModuleDeps
-  }
+  def recursiveModuleDeps: Seq[JavaModule] = { recModuleDeps }
+
+  /** The direct and indirect runtime module dependencies of this module */
+  def recursiveRunModuleDeps: Seq[JavaModule] = { recRunModuleDeps }
 
   /**
    * Like `recursiveModuleDeps` but also include the module itself,
    * basically the modules whose classpath are needed at runtime
    */
-  def transitiveModuleDeps: Seq[JavaModule] = Seq(this) ++ recursiveModuleDeps
+  def transitiveModuleDeps: Seq[JavaModule] = recursiveModuleDeps ++ Seq(this)
+
+  /**
+   * Like `recursiveModuleDeps` but also include the module itself,
+   * basically the modules whose classpath are needed at runtime
+   */
+  def transitiveRunModuleDeps: Seq[JavaModule] = recursiveRunModuleDeps ++ Seq(this)
 
   /**
    * All direct and indirect module dependencies of this module, including
@@ -240,6 +273,17 @@ trait JavaModule
     (moduleDepsChecked ++ compileModuleDepsChecked).flatMap(_.transitiveModuleDeps).distinct
   }
 
+  /**
+   * All direct and indirect module dependencies of this module, including
+   * compile-only dependencies: basically the modules whose classpath are needed
+   * at runtime.
+   *
+   * Note that `runModuleDeps` are defined to be transitive
+   */
+  def transitiveModuleRunModuleDeps: Seq[JavaModule] = {
+    (runModuleDepsChecked ++ moduleDepsChecked).flatMap(_.transitiveRunModuleDeps).distinct
+  }
+
   /** The compile-only transitive ivy dependencies of this module and all it's upstream compile-only modules. */
   def transitiveCompileIvyDeps: T[Agg[BoundDep]] = Task {
     // We never include compile-only dependencies transitively, but we must include normal transitive dependencies!
@@ -287,6 +331,17 @@ trait JavaModule
       T.traverse(moduleDepsChecked)(_.transitiveIvyDeps)().flatten
   }
 
+  /**
+   * The transitive run ivy dependencies of this module and all it's upstream modules.
+   * This is calculated from [[runIvyDeps]], [[mandatoryIvyDeps]] and recursively from [[moduleDeps]].
+   */
+  def transitiveRunIvyDeps: T[Agg[BoundDep]] = Task {
+    runIvyDeps().map(bindDependency()) ++
+      T.traverse(moduleDepsChecked)(_.transitiveRunIvyDeps)().flatten ++
+      T.traverse(runModuleDepsChecked)(_.transitiveIvyDeps)().flatten ++
+      T.traverse(runModuleDepsChecked)(_.transitiveRunIvyDeps)().flatten
+  }
+
   /**
    * The upstream compilation output of all this module's upstream modules
    */
@@ -298,7 +353,7 @@ trait JavaModule
    * The transitive version of `localClasspath`
    */
   def transitiveLocalClasspath: T[Agg[PathRef]] = Task {
-    T.traverse(transitiveModuleCompileModuleDeps)(_.localClasspath)().flatten
+    T.traverse(transitiveModuleRunModuleDeps)(_.localClasspath)().flatten
   }
 
   /**
@@ -493,7 +548,7 @@ trait JavaModule
    * excluding upstream modules and third-party dependencies, but including unmanaged dependencies.
    *
    * This is build from [[localCompileClasspath]] and [[localRunClasspath]]
-   * as the parts available "before compilation" and "after compiliation".
+   * as the parts available "before compilation" and "after compilation".
    *
    * Keep in sync with [[bspLocalClasspath]]
    */
@@ -563,7 +618,7 @@ trait JavaModule
 
   def resolvedRunIvyDeps: T[Agg[PathRef]] = Task {
     defaultResolver().resolveDeps(
-      runIvyDeps().map(bindDependency()) ++ transitiveIvyDeps(),
+      transitiveRunIvyDeps() ++ transitiveIvyDeps(),
       artifactTypes = Some(artifactTypes())
     )
   }
@@ -877,7 +932,7 @@ trait JavaModule
             printDepsTree(
               args.inverse.value,
               Task.Anon {
-                transitiveCompileIvyDeps() ++ runIvyDeps().map(bindDependency())
+                transitiveCompileIvyDeps() ++ transitiveRunIvyDeps()
               },
               validModules
             )()
@@ -890,7 +945,7 @@ trait JavaModule
           Task.Command {
             printDepsTree(
               args.inverse.value,
-              Task.Anon { runIvyDeps().map(bindDependency()) },
+              Task.Anon { transitiveRunIvyDeps() },
               validModules
             )()
           }
@@ -1046,7 +1101,7 @@ trait JavaModule
         },
         Task.Anon {
           defaultResolver().resolveDeps(
-            runIvyDeps().map(bindDependency()) ++ transitiveIvyDeps(),
+            transitiveRunIvyDeps() ++ transitiveIvyDeps(),
             sources = true
           )
         }

scalalib/src/mill/scalalib/ScalaModule.scala

@@ -481,7 +481,7 @@ trait ScalaModule extends JavaModule with TestModule.ScalaModuleBase { outer =>
         )
       }
       val bind = bindDependency()
-      runIvyDeps().map(bind) ++ transitiveIvyDeps() ++
+      transitiveRunIvyDeps() ++ transitiveIvyDeps() ++
         Agg(ivy"com.lihaoyi:::ammonite:${ammVersion}").map(bind)
     }
   }

scalalib/test/src/mill/scalalib/ScalaIvyDepsTests.scala

@@ -17,6 +17,30 @@ object ScalaIvyDepsTests extends TestSuite {
     }
   }
 
+  object TransitiveRunIvyDeps extends TestBaseModule {
+    object upstream extends JavaModule {
+      def ivyDeps = Agg(ivy"org.slf4j:slf4j-api:2.0.16")
+      def runIvyDeps = Agg(ivy"ch.qos.logback:logback-classic:1.5.10")
+    }
+
+    object downstream extends JavaModule {
+      // Make sure runIvyDeps are transitively picked up from normal `moduleDeps`
+      def moduleDeps = Seq(upstream)
+    }
+  }
+
+  object TransitiveRunIvyDeps2 extends TestBaseModule {
+    object upstream extends JavaModule {
+      def ivyDeps = Agg(ivy"org.slf4j:slf4j-api:2.0.16")
+      def runIvyDeps = Agg(ivy"ch.qos.logback:logback-classic:1.5.10")
+    }
+
+    object downstream extends JavaModule {
+      // Make sure both ivyDeps and runIvyDeps are transitively picked up from `runModuleDeps`
+      def runModuleDeps = Seq(upstream)
+    }
+  }
+
   def tests: Tests = Tests {
 
     test("ivyDeps") - UnitTester(HelloWorldIvyDeps, resourcePath).scoped { eval =>
@@ -33,5 +57,23 @@ object ScalaIvyDepsTests extends TestSuite {
       )
     }
 
+    test("transitiveRun") - UnitTester(TransitiveRunIvyDeps, resourcePath).scoped { eval =>
+      val Right(result2) = eval.apply(TransitiveRunIvyDeps.downstream.runClasspath)
+
+      assert(
+        result2.value.exists(_.path.last == "logback-classic-1.5.10.jar")
+      )
+    }
+
+    test("transitiveLocalRuntimeDepsRun") - UnitTester(TransitiveRunIvyDeps2, resourcePath).scoped {
+      eval =>
+        val Right(result2) = eval.apply(TransitiveRunIvyDeps2.downstream.runClasspath)
+
+        assert(
+          result2.value.exists(_.path.last == "logback-classic-1.5.10.jar"),
+          result2.value.exists(_.path.last == "slf4j-api-2.0.16.jar")
+        )
+    }
+
   }
 }