From 807814bc6b95d32b3208d94971ff4bbb1171fa76 Mon Sep 17 00:00:00 2001 From: Tobias Roeser Date: Wed, 20 Sep 2023 15:17:05 +0200 Subject: [PATCH 1/3] Introduce a MillException to transport the error condition --- main/api/src/mill/api/MillException.scala | 7 ++++++ runner/src/mill/runner/MillMain.scala | 27 ++++++++++++++++++--- runner/src/mill/runner/MillServerMain.scala | 23 +++++++++--------- 3 files changed, 43 insertions(+), 14 deletions(-) create mode 100644 main/api/src/mill/api/MillException.scala diff --git a/main/api/src/mill/api/MillException.scala b/main/api/src/mill/api/MillException.scala new file mode 100644 index 00000000000..e6de077d784 --- /dev/null +++ b/main/api/src/mill/api/MillException.scala @@ -0,0 +1,7 @@ +package mill.api + +/** + * This exception is specifically handled in [[mill.runner.MillMain]] and [[mill.runner.MillServerMain]]. You can use it, if you need to exit Mill with a nice error message. + * @param msg The error message, to be displayed to the user. + */ +class MillException(msg: String) extends Exception(msg) diff --git a/runner/src/mill/runner/MillMain.scala b/runner/src/mill/runner/MillMain.scala index 47ab7efe8e0..65dfb41f88f 100644 --- a/runner/src/mill/runner/MillMain.scala +++ b/runner/src/mill/runner/MillMain.scala @@ -1,18 +1,38 @@ package mill.runner -import mill.main.BuildInfo import java.io.{FileOutputStream, PrintStream} import java.util.Locale import scala.jdk.CollectionConverters._ import scala.util.Properties import mill.java9rtexport.Export -import mill.api.{DummyInputStream, internal} -import mill.api.SystemStreams +import mill.api.{DummyInputStream, MillException, internal, SystemStreams} import mill.bsp.{BspContext, BspServerResult} +import mill.main.BuildInfo import mill.util.PrintLogger +import java.lang.reflect.InvocationTargetException +import scala.util.control.NonFatal + @internal object MillMain { + + def handleMillException[T]( + err: PrintStream, + onError: => T + ): PartialFunction[Throwable, (Boolean, T)] = { + case e: MillException => + err.println(e.getMessage()) + (false, onError) + case e: InvocationTargetException + if e.getCause != null && e.getCause.isInstanceOf[MillException] => + err.println(e.getCause.getMessage()) + (false, onError) + case NonFatal(e) => + err.println("An unexpected error occurred") + throw e + (false, onError) + } + def main(args: Array[String]): Unit = { val initialSystemStreams = new SystemStreams(System.out, System.err, System.in) // setup streams @@ -57,6 +77,7 @@ object MillMain { userSpecifiedProperties0 = Map(), initialSystemProperties = sys.props.toMap ) + catch handleMillException(runnerStreams.err, ()) finally { cleanupStreams.foreach(_.close()) } diff --git a/runner/src/mill/runner/MillServerMain.scala b/runner/src/mill/runner/MillServerMain.scala index da2f0b807f5..ea2b2702058 100644 --- a/runner/src/mill/runner/MillServerMain.scala +++ b/runner/src/mill/runner/MillServerMain.scala @@ -71,17 +71,18 @@ object MillServerMain extends MillServerMain[RunnerState] { userSpecifiedProperties: Map[String, String], initialSystemProperties: Map[String, String] ): (Boolean, RunnerState) = { - MillMain.main0( - args = args, - stateCache = stateCache, - mainInteractive = mainInteractive, - streams0 = streams, - bspLog = None, - env = env, - setIdle = setIdle, - userSpecifiedProperties0 = userSpecifiedProperties, - initialSystemProperties = initialSystemProperties - ) + try MillMain.main0( + args = args, + stateCache = stateCache, + mainInteractive = mainInteractive, + streams0 = streams, + bspLog = None, + env = env, + setIdle = setIdle, + userSpecifiedProperties0 = userSpecifiedProperties, + initialSystemProperties = initialSystemProperties + ) + catch MillMain.handleMillException(streams.err, stateCache) } } From c72567e9fd86ef97b3d18a7801c5cffef446584b Mon Sep 17 00:00:00 2001 From: Tobias Roeser Date: Mon, 6 Mar 2023 09:15:57 +0100 Subject: [PATCH 2/3] WIP: cycle detection , issue https://github.com/com-lihaoyi/mill/issues/2341 --- .../src/mill/contrib/bloop/BloopImpl.scala | 5 +- main/api/src/mill/api/MillException.scala | 3 + main/define/src/mill/define/Task.scala | 2 +- scalalib/src/mill/scalalib/GenIdeaImpl.scala | 4 +- scalalib/src/mill/scalalib/JavaModule.scala | 59 +++++++++++++---- .../src/mill/scalalib/PublishModule.scala | 6 +- .../scalalib/internal/JavaModuleUtils.scala | 2 +- .../mill/scalalib/internal/ModuleUtils.scala | 37 +++++++++++ .../test/src/mill/scalalib/CycleTests.scala | 65 +++++++++++++++++++ 9 files changed, 163 insertions(+), 20 deletions(-) create mode 100644 scalalib/test/src/mill/scalalib/CycleTests.scala diff --git a/contrib/bloop/src/mill/contrib/bloop/BloopImpl.scala b/contrib/bloop/src/mill/contrib/bloop/BloopImpl.scala index 8859c478ea2..2e7e8308807 100644 --- a/contrib/bloop/src/mill/contrib/bloop/BloopImpl.scala +++ b/contrib/bloop/src/mill/contrib/bloop/BloopImpl.scala @@ -201,7 +201,7 @@ class BloopImpl(ev: () => Evaluator, wd: os.Path) extends ExternalModule { outer // ////////////////////////////////////////////////////////////////////////// val classpath = T.task { - val depModules = (module.compileModuleDeps ++ module.recursiveModuleDeps).distinct + val depModules = (module.compileModuleDepsChecked ++ module.recursiveModuleDeps).distinct // dep modules ++ ivy deps ++ unmanaged depModules.map(classes) ++ module.resolvedIvyDeps().map(_.path) ++ @@ -408,7 +408,8 @@ class BloopImpl(ev: () => Evaluator, wd: os.Path) extends ExternalModule { outer sources = mSources, sourcesGlobs = None, sourceRoots = None, - dependencies = (module.moduleDeps ++ module.compileModuleDeps).map(name).toList, + dependencies = + (module.moduleDepsChecked ++ module.compileModuleDepsChecked).map(name).toList, classpath = classpath().map(_.toNIO).toList, out = out(module).toNIO, classesDir = classes(module).toNIO, diff --git a/main/api/src/mill/api/MillException.scala b/main/api/src/mill/api/MillException.scala index e6de077d784..94b143ee307 100644 --- a/main/api/src/mill/api/MillException.scala +++ b/main/api/src/mill/api/MillException.scala @@ -5,3 +5,6 @@ package mill.api * @param msg The error message, to be displayed to the user. */ class MillException(msg: String) extends Exception(msg) + +class BuildScriptException(msg: String) + extends MillException("Build script contains errors:\n" + msg) diff --git a/main/define/src/mill/define/Task.scala b/main/define/src/mill/define/Task.scala index 7b2a949aa22..10e9970dd18 100644 --- a/main/define/src/mill/define/Task.scala +++ b/main/define/src/mill/define/Task.scala @@ -354,7 +354,7 @@ object Target extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] { val taskIsPrivate = isPrivateTargetOption(c) - val lhs = Applicative.impl0[Task, T, mill.api.Ctx](c)(reify(Result.Success(t.splice)).tree) + val lhs = Applicative.impl0[Task, T, mill.api.Ctx](c)(reify(Result.create(t.splice)).tree) mill.moduledefs.Cacher.impl0[Target[T]](c)( reify( diff --git a/scalalib/src/mill/scalalib/GenIdeaImpl.scala b/scalalib/src/mill/scalalib/GenIdeaImpl.scala index a7d357b9bc3..61599825bcb 100755 --- a/scalalib/src/mill/scalalib/GenIdeaImpl.scala +++ b/scalalib/src/mill/scalalib/GenIdeaImpl.scala @@ -554,8 +554,8 @@ case class GenIdeaImpl( val libNames = Strict.Agg.from(sanizedDeps).iterator.toSeq val depNames = Strict.Agg - .from(mod.moduleDeps.map((_, None)) ++ - mod.compileModuleDeps.map((_, Some("PROVIDED")))) + .from(mod.moduleDepsChecked.map((_, None)) ++ + mod.compileModuleDepsChecked.map((_, Some("PROVIDED")))) .filter(!_._1.skipIdea) .map { case (v, s) => ScopedOrd(moduleName(moduleLabels(v)), s) } .iterator diff --git a/scalalib/src/mill/scalalib/JavaModule.scala b/scalalib/src/mill/scalalib/JavaModule.scala index 609fb8014b3..439d14f8b93 100644 --- a/scalalib/src/mill/scalalib/JavaModule.scala +++ b/scalalib/src/mill/scalalib/JavaModule.scala @@ -9,13 +9,14 @@ import coursier.parse.JavaOrScalaModule import coursier.parse.ModuleParser import coursier.util.ModuleMatcher import mainargs.Flag -import mill.api.Loose.Agg -import mill.define.ModuleRef +import mill.Agg import mill.api.{JarManifest, PathRef, Result, internal} -import mill.util.Jvm +import mill.define.{Command, ModuleRef, Segment, Task, TaskModule} +import mill.scalalib.internal.ModuleUtils import mill.scalalib.api.CompilationResult import mill.scalalib.bsp.{BspBuildTarget, BspModule} import mill.scalalib.publish.Artifact +import mill.util.Jvm import os.Path /** @@ -122,15 +123,49 @@ trait JavaModule */ def javacOptions: T[Seq[String]] = T { Seq.empty[String] } - /** The direct dependencies of this module */ + /** + * The direct dependencies of this module. + * @see [[moduleDepschecked]] + */ def moduleDeps: Seq[JavaModule] = Seq.empty + /** Same as [[moduleDeps]] but checked to not contain cycles. */ + final def moduleDepsChecked: Seq[JavaModule] = { + // trigger initialization to check for cycles + recModuleDeps + moduleDeps + } + + /** Should only be called from [[moduleDepsChecked]] */ + private lazy val recModuleDeps: Seq[JavaModule] = + ModuleUtils.recursive[JavaModule]( + (millModuleSegments ++ Seq(Segment.Label("moduleDeps"))).render, + this, + _.moduleDeps + ) + /** The compile-only direct dependencies of this module. */ def compileModuleDeps: Seq[JavaModule] = Seq.empty + /** Same as [[compileModuleDeps]] but checked to not contain cycles. */ + final def compileModuleDepsChecked: Seq[JavaModule] = { + // trigger initialization to check for cycles + recCompileModuleDeps + compileModuleDeps + } + + /** Should only be called from [[compileModuleDeps]] */ + private lazy val recCompileModuleDeps: Seq[JavaModule] = + ModuleUtils.recursive[JavaModule]( + (millModuleSegments ++ Seq(Segment.Label("compileModuleDeps"))).render, + this, + _.compileModuleDeps + ) + /** The direct and indirect dependencies of this module */ def recursiveModuleDeps: Seq[JavaModule] = { - moduleDeps.flatMap(_.transitiveModuleDeps).distinct +// moduleDeps.flatMap(_.transitiveModuleDeps).distinct + recModuleDeps } /** @@ -148,14 +183,14 @@ trait JavaModule * look at the direct `compileModuleDeps` when assembling this list */ def transitiveModuleCompileModuleDeps: Seq[JavaModule] = { - (moduleDeps ++ compileModuleDeps).flatMap(_.transitiveModuleDeps).distinct + (moduleDepsChecked ++ compileModuleDepsChecked).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 + T.traverse(compileModuleDepsChecked)(_.transitiveIvyDeps)().flatten } /** @@ -163,17 +198,17 @@ trait JavaModule * @param recursive If `true` include all recursive module dependencies, else only show direct dependencies. */ def showModuleDeps(recursive: Boolean = false): Command[Unit] = T.command { - val normalDeps = if (recursive) recursiveModuleDeps else moduleDeps + val normalDeps = if (recursive) recursiveModuleDeps else moduleDepsChecked val compileDeps = - if (recursive) compileModuleDeps.flatMap(_.transitiveModuleDeps).distinct - else compileModuleDeps + if (recursive) compileModuleDepsChecked.flatMap(_.transitiveModuleDeps).distinct + else compileModuleDepsChecked val deps = (normalDeps ++ compileDeps).distinct val asString = s"${if (recursive) "Recursive module" else "Module"} dependencies of ${millModuleSegments.render}:\n\t${deps .map { dep => dep.millModuleSegments.render ++ - (if (compileModuleDeps.contains(dep) || !normalDeps.contains(dep)) " (compile)" + (if (compileModuleDepsChecked.contains(dep) || !normalDeps.contains(dep)) " (compile)" else "") } .mkString("\n\t")}" @@ -193,7 +228,7 @@ trait JavaModule */ def transitiveIvyDeps: T[Agg[BoundDep]] = T { (ivyDeps() ++ mandatoryIvyDeps()).map(bindDependency()) ++ - T.traverse(moduleDeps)(_.transitiveIvyDeps)().flatten + T.traverse(moduleDepsChecked)(_.transitiveIvyDeps)().flatten } /** diff --git a/scalalib/src/mill/scalalib/PublishModule.scala b/scalalib/src/mill/scalalib/PublishModule.scala index 9fd339707b4..ffa1b57da18 100644 --- a/scalalib/src/mill/scalalib/PublishModule.scala +++ b/scalalib/src/mill/scalalib/PublishModule.scala @@ -58,8 +58,10 @@ trait PublishModule extends JavaModule { outer => .filter(!ivyPomDeps.contains(_)) .map(_.copy(scope = Scope.Provided)) - val modulePomDeps = T.sequence(moduleDeps.map(_.publishSelfDependency))() - val compileModulePomDeps = T.sequence(compileModuleDeps.collect { + val modulePomDeps = T.sequence(moduleDepsChecked.collect { + case m: PublishModule => m.publishSelfDependency + })() + val compileModulePomDeps = T.sequence(compileModuleDepsChecked.collect { case m: PublishModule => m.publishSelfDependency })() diff --git a/scalalib/src/mill/scalalib/internal/JavaModuleUtils.scala b/scalalib/src/mill/scalalib/internal/JavaModuleUtils.scala index 51a6a41863d..aafd46c4afa 100644 --- a/scalalib/src/mill/scalalib/internal/JavaModuleUtils.scala +++ b/scalalib/src/mill/scalalib/internal/JavaModuleUtils.scala @@ -15,7 +15,7 @@ object JavaModuleUtils { found else { val subMods = mod.millModuleDirectChildren ++ (mod match { - case jm: JavaModule => jm.moduleDeps ++ jm.compileModuleDeps + case jm: JavaModule => jm.moduleDepsChecked ++ jm.compileModuleDepsChecked case other => Seq.empty }) subMods.foldLeft(found ++ Seq(mod)) { (all, mod) => loop(mod, all) } diff --git a/scalalib/src/mill/scalalib/internal/ModuleUtils.scala b/scalalib/src/mill/scalalib/internal/ModuleUtils.scala index 491be9ec67b..44cfd244285 100644 --- a/scalalib/src/mill/scalalib/internal/ModuleUtils.scala +++ b/scalalib/src/mill/scalalib/internal/ModuleUtils.scala @@ -1,7 +1,10 @@ package mill.scalalib.internal +import mill.api.{BuildScriptException, experimental} import mill.define.{Module, Segments} +import scala.annotation.tailrec + @mill.api.internal object ModuleUtils { @@ -11,4 +14,38 @@ object ModuleUtils { def moduleDisplayName(module: Module): String = { (module.millModuleShared.value.getOrElse(Segments()) ++ module.millModuleSegments).render } + + def recursive[T <: Module](name: String, start: T, deps: T => Seq[T]): Seq[T] = { + + @tailrec def rec( + seenModules: List[T], + toAnalyze: List[(List[T], List[T])] + ): List[T] = { + toAnalyze match { + case Nil => seenModules + case traces :: rest => + traces match { + case (_, Nil) => rec(seenModules, rest) + case (trace, cand :: remaining) => + if (trace.contains(cand)) { + // cycle! + val rendered = + (cand :: (cand :: trace.takeWhile(_ != cand)).reverse).mkString(" -> ") + val msg = s"${name}: cycle detected: ${rendered}" + println(msg) + throw new BuildScriptException(msg) + } + rec( + seenModules ++ Seq(cand), + toAnalyze = ((cand :: trace, deps(cand).toList)) :: (trace, remaining) :: rest + ) + } + } + } + + rec( + seenModules = List(), + toAnalyze = List((List(start), deps(start).toList)) + ).reverse + } } diff --git a/scalalib/test/src/mill/scalalib/CycleTests.scala b/scalalib/test/src/mill/scalalib/CycleTests.scala new file mode 100644 index 00000000000..22c888d7323 --- /dev/null +++ b/scalalib/test/src/mill/scalalib/CycleTests.scala @@ -0,0 +1,65 @@ +package mill.scalalib + +import mill.api.BuildScriptException +import mill.util.{TestEvaluator, TestUtil} +import utest.framework.TestPath +import utest.{TestSuite, Tests, compileError, intercept, test, assert} + +object CycleTests extends TestSuite { + + object CycleBase extends TestUtil.BaseModule { + // See issue: https://github.com/com-lihaoyi/mill/issues/2341 + object a extends ScalaModule { + override def moduleDeps = Seq(a) + override def scalaVersion = sys.props.getOrElse("TEST_SCALA_VERSION", ???) + } + object b extends JavaModule { + override def moduleDeps = Seq(c) + object c extends JavaModule { + override def moduleDeps = Seq(d) + } + object d extends JavaModule { + override def moduleDeps = Seq(b) + } + } + object e extends JavaModule { + override def moduleDeps = Seq(b) + } + object f extends JavaModule { + override def compileModuleDeps = Seq(f) + } + } + + def workspaceTest[T](m: TestUtil.BaseModule)(t: TestEvaluator => T)(implicit tp: TestPath): T = { + val eval = new TestEvaluator(m) + os.remove.all(m.millSourcePath) + os.remove.all(eval.outPath) + os.makeDir.all(m.millSourcePath / os.up) + t(eval) + } + + override def tests: Tests = Tests { + test("moduleDeps") { + test("self-reference") - workspaceTest(CycleBase) { eval => + val ex = intercept[BuildScriptException] { + eval.apply(CycleBase.a.compile) + } + assert(ex.getMessage.contains("a.moduleDeps: cycle detected: a -> a")) + } + test("cycle-in-deps") - workspaceTest(CycleBase) { eval => + val ex = intercept[BuildScriptException] { + eval.apply(CycleBase.e.compile) + } + assert(ex.getMessage.contains("e.moduleDeps: cycle detected: b -> b.c -> b.d -> b")) + } + } + test("compileModuleDeps") { + test("self-reference") - workspaceTest(CycleBase) { eval => + val ex = intercept[BuildScriptException] { + eval.apply(CycleBase.f.compile) + } + assert(ex.getMessage.contains("f.compileModuleDeps: cycle detected: f -> f")) + } + } + } +} From 7b92c846475712d1b6d53d6dd4111fbef8455213 Mon Sep 17 00:00:00 2001 From: Tobias Roeser Date: Wed, 27 Sep 2023 16:34:13 +0200 Subject: [PATCH 3/3] Removed unrealistic deeply nested module tests --- .../test/src/mill/scalalib/bsp/BspModuleTests.scala | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/scalalib/test/src/mill/scalalib/bsp/BspModuleTests.scala b/scalalib/test/src/mill/scalalib/bsp/BspModuleTests.scala index 0fff6a7f71b..c126f3c58cf 100644 --- a/scalalib/test/src/mill/scalalib/bsp/BspModuleTests.scala +++ b/scalalib/test/src/mill/scalalib/bsp/BspModuleTests.scala @@ -31,7 +31,7 @@ object BspModuleTests extends TestSuite { } object InterDeps extends BspBase { - val maxCrossCount = 25 + val maxCrossCount = 15 val configs = 1.to(maxCrossCount) object Mod extends Cross[ModCross](configs) trait ModCross extends ScalaModule with Cross.Module[Int] { @@ -105,7 +105,7 @@ object BspModuleTests extends TestSuite { } test("interdependencies are fast") { test("reference (no BSP)") { - def runNoBsp(entry: Int, maxTime: Int) = workspaceTest(MultiBase) { eval => + def runNoBsp(entry: Int, maxTime: Int) = workspaceTest(InterDeps) { eval => val start = System.currentTimeMillis() val Right((result, evalCount)) = eval.apply( InterDeps.Mod(entry).compileClasspath @@ -116,11 +116,10 @@ object BspModuleTests extends TestSuite { } test("index 1 (no deps)") { runNoBsp(1, 5000) } test("index 10") { runNoBsp(10, 30000) } - test("index 20") { runNoBsp(20, 30000) } - test("index 25") { runNoBsp(25, 100000) } + test("index 15") { runNoBsp(15, 30000) } } def run(entry: Int, maxTime: Int) = retry(3) { - workspaceTest(MultiBase) { eval => + workspaceTest(InterDeps) { eval => val start = System.currentTimeMillis() val Right((result, evalCount)) = eval.apply( InterDeps.Mod(entry).bspCompileClasspath @@ -132,8 +131,7 @@ object BspModuleTests extends TestSuite { } test("index 1 (no deps)") { run(1, 500) } test("index 10") { run(10, 5000) } - test("index 20") { run(20, 15000) } - test("index 25") { run(25, 50000) } + test("index 15") { run(15, 15000) } } } }