Skip to content

Commit

Permalink
Remove adding test options to the project/build target name hash (#3107)
Browse files Browse the repository at this point in the history
* 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.

* Apply scalafix

* Add test for the number of build targets created when changing options from cli and from sources

* Format
  • Loading branch information
MaciejG604 authored Aug 21, 2024
1 parent 58f5a76 commit 6826e67
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 38 deletions.
26 changes: 9 additions & 17 deletions modules/build/src/main/scala/scala/build/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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("")
)
val optionsHash = options.hash

inputs.copy(baseProjectName = inputs.baseProjectName + optionsHash.fold("")("_" + _))
}

private def allInputs(
Expand Down Expand Up @@ -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))
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -697,4 +676,47 @@ 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)
}
}
}

0 comments on commit 6826e67

Please sign in to comment.