From c4a3cf3ede7811909d665d83eb0cd97ae74e98d1 Mon Sep 17 00:00:00 2001 From: Maciej Gajek Date: Thu, 15 Aug 2024 14:48:33 +0200 Subject: [PATCH 1/4] Remove adding test options to the project/build target name hash - this should not be put there. The hash in the build target name is used to introduce hashing when buildOptions are changed from the CLI. Otherwise, if options are changed from sources, Bloop recompiles the project anyway because the sources changed. Adding this new hash that comes from sources possibly creates new build targets every time somebody fiddles with some test scope options. It's just nonsense. Also look at the comments in the code, we've made a rule and then stopped following it. --- .../src/main/scala/scala/build/Build.scala | 22 ++++++------------- .../integration/CompileTestDefinitions.scala | 21 ------------------ 2 files changed, 7 insertions(+), 36 deletions(-) diff --git a/modules/build/src/main/scala/scala/build/Build.scala b/modules/build/src/main/scala/scala/build/Build.scala index 332a7da206..0852cc660f 100644 --- a/modules/build/src/main/scala/scala/build/Build.scala +++ b/modules/build/src/main/scala/scala/build/Build.scala @@ -200,22 +200,15 @@ object Build { */ def updateInputs( inputs: Inputs, - options: BuildOptions, - testOptions: Option[BuildOptions] = None + options: BuildOptions ): Inputs = { // If some options are manually overridden, append a hash of the options to the project name // Using options, not options0 - only the command-line options are taken into account. No hash is // appended for options from the sources. val optionsHash = options.hash - val testOptionsHash = testOptions.flatMap(_.hash) - inputs.copy( - baseProjectName = - inputs.baseProjectName - + optionsHash.map("_" + _).getOrElse("") - + testOptionsHash.map("_" + _).getOrElse("") - ) + inputs.copy(baseProjectName = inputs.baseProjectName + optionsHash.fold("")("_" + _)) } private def allInputs( @@ -278,6 +271,11 @@ object Build { overrideOptions: BuildOptions ): Either[BuildException, NonCrossBuilds] = either { + val inputs0 = updateInputs( + inputs, + overrideOptions.orElse(options) // update hash in inputs with options coming from the CLI or cross-building, not from the sources + ) + val baseOptions = overrideOptions.orElse(sharedOptions) val scopedSources = value(crossSources.scopedSources(baseOptions)) @@ -290,12 +288,6 @@ object Build { value(scopedSources.sources(Scope.Test, baseOptions, inputs.workspace, logger)) val testOptions = testSources.buildOptions - val inputs0 = updateInputs( - inputs, - mainOptions, // update hash in inputs with options coming from the CLI or cross-building, not from the sources - Some(testOptions).filter(_ != mainOptions) - ) - def doBuildScope( options: BuildOptions, sources: Sources, diff --git a/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala b/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala index f393df0a46..50456e6199 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala @@ -183,16 +183,7 @@ abstract class CompileTestDefinitions test("no arg") { simpleInputs.fromRoot { root => - val projectFilePrefix = root.baseName + "_" os.proc(TestUtil.cli, "compile", extraOptions, ".").call(cwd = root) - val projDirs = os.list(root / Constants.workspaceDirName) - .filter(_.last.startsWith(projectFilePrefix)) - .filter(os.isDir(_)) - expect(projDirs.length == 1) - val projDir = projDirs.head - val projDirName = projDir.last - val elems = projDirName.stripPrefix(projectFilePrefix).split("[-_]").toSeq - expect(elems.length == 1) } } @@ -254,18 +245,6 @@ abstract class CompileTestDefinitions ) expect(isDefinedTestPathInClassPath) checkIfCompileOutputIsCopied("Tests", tempOutput) - - val projectFilePrefix = root.baseName + "_" - - val projDirs = os.list(root / Constants.workspaceDirName) - .filter(_.last.startsWith(projectFilePrefix)) - .filter(os.isDir(_)) - expect(projDirs.length == 1) - val projDir = projDirs.head - val projDirName = projDir.last - val elems = projDirName.stripPrefix(projectFilePrefix).split("[-_]").toSeq - expect(elems.length == 2) - expect(elems.toSet.size == 2) } } From 6f7feb69e6ba32c2c3161237edec99b521728da8 Mon Sep 17 00:00:00 2001 From: Maciej Gajek Date: Mon, 19 Aug 2024 10:25:50 +0200 Subject: [PATCH 2/4] Apply scalafix --- modules/build/src/main/scala/scala/build/Build.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/build/src/main/scala/scala/build/Build.scala b/modules/build/src/main/scala/scala/build/Build.scala index 0852cc660f..95674c7118 100644 --- a/modules/build/src/main/scala/scala/build/Build.scala +++ b/modules/build/src/main/scala/scala/build/Build.scala @@ -206,7 +206,7 @@ object Build { // If some options are manually overridden, append a hash of the options to the project name // Using options, not options0 - only the command-line options are taken into account. No hash is // appended for options from the sources. - val optionsHash = options.hash + val optionsHash = options.hash inputs.copy(baseProjectName = inputs.baseProjectName + optionsHash.fold("")("_" + _)) } @@ -275,7 +275,7 @@ object Build { inputs, overrideOptions.orElse(options) // update hash in inputs with options coming from the CLI or cross-building, not from the sources ) - + val baseOptions = overrideOptions.orElse(sharedOptions) val scopedSources = value(crossSources.scopedSources(baseOptions)) From 377948adf4a8011a74c3f4ee4ddddec3ae357183 Mon Sep 17 00:00:00 2001 From: Maciej Gajek Date: Tue, 20 Aug 2024 19:48:35 +0200 Subject: [PATCH 3/4] Add test for the number of build targets created when changing options from cli and from sources --- .../integration/CompileTestDefinitions.scala | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala b/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala index 50456e6199..1dfdedd39b 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala @@ -676,4 +676,43 @@ abstract class CompileTestDefinitions expect(out.contains("Too small maximum heap")) } } + + test("new build targets should only be created when CLI options change") { + val filename = "Main.scala" + val inputs = TestInputs( + os.rel / filename -> + """object Main extends App { + | println("Hello") + |} + |""".stripMargin, + + os.rel / "Test.test.scala" -> + """object Test extends App { + | println("Hello") + |} + |""".stripMargin + ) + inputs.fromRoot { root => + os.proc(TestUtil.cli, "compile", extraOptions :+ "--test", ".").call(cwd = root) + + def buildTargetDirs = os.list(root / Constants.workspaceDirName) + .filter(os.isDir) + .filter(_.last != ".bloop") + + expect(buildTargetDirs.size == 1) + + os.write.over(root/filename, """//> using dep com.lihaoyi::os-lib:0.9.1 + | + |object Main extends App { + | println("Hello") + |} + |""".stripMargin) + + os.proc(TestUtil.cli, "compile", extraOptions :+ "--test", ".").call(cwd = root) + expect(buildTargetDirs.size == 1) + + os.proc(TestUtil.cli, "compile", extraOptions ++ Seq("--test", "-nowarn"), ".").call(cwd = root) + expect(buildTargetDirs.size == 2) + } + } } From 3761f4b4bdeae2482dc4d3b375c2b402b6c02279 Mon Sep 17 00:00:00 2001 From: Maciej Gajek Date: Tue, 20 Aug 2024 23:04:43 +0200 Subject: [PATCH 4/4] Format --- .../integration/CompileTestDefinitions.scala | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala b/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala index 1dfdedd39b..c60703d020 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala @@ -685,7 +685,6 @@ abstract class CompileTestDefinitions | println("Hello") |} |""".stripMargin, - os.rel / "Test.test.scala" -> """object Test extends App { | println("Hello") @@ -701,17 +700,22 @@ abstract class CompileTestDefinitions expect(buildTargetDirs.size == 1) - os.write.over(root/filename, """//> using dep com.lihaoyi::os-lib:0.9.1 - | - |object Main extends App { - | println("Hello") - |} - |""".stripMargin) + os.write.over( + root / filename, + """//> using dep com.lihaoyi::os-lib:0.9.1 + | + |object Main extends App { + | println("Hello") + |} + |""".stripMargin + ) os.proc(TestUtil.cli, "compile", extraOptions :+ "--test", ".").call(cwd = root) expect(buildTargetDirs.size == 1) - os.proc(TestUtil.cli, "compile", extraOptions ++ Seq("--test", "-nowarn"), ".").call(cwd = root) + os.proc(TestUtil.cli, "compile", extraOptions ++ Seq("--test", "-nowarn"), ".").call(cwd = + root + ) expect(buildTargetDirs.size == 2) } }