-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
avoid full (re)compilation after cold scalafix invocations #411
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -102,9 +102,20 @@ object ScalafixPlugin extends AutoPlugin { | |||||||||||||||||
val oldCompile = | ||||||||||||||||||
compile.value // evaluated first, before the potential scalafix evaluation | ||||||||||||||||||
if (scalafixOnCompile.value) | ||||||||||||||||||
scalafix | ||||||||||||||||||
.toTask(" --triggered") | ||||||||||||||||||
.map(_ => oldCompile) | ||||||||||||||||||
Def.taskDyn { | ||||||||||||||||||
// evaluate scalafixInvoked only when scalafixOnCompile is set | ||||||||||||||||||
// to reduce risk of side effects on the build | ||||||||||||||||||
if (scalafixInvoked.value) | ||||||||||||||||||
// don't trigger scalafix if it was explicitly invoked, | ||||||||||||||||||
// as it would be not only inefficient, but incorrect since | ||||||||||||||||||
// scalafix might run with different CLI args, potentially | ||||||||||||||||||
// rewriting files while `--check` was used | ||||||||||||||||||
Def.task(oldCompile) | ||||||||||||||||||
else | ||||||||||||||||||
scalafix | ||||||||||||||||||
.toTask(" --triggered") | ||||||||||||||||||
.map(_ => oldCompile) | ||||||||||||||||||
} | ||||||||||||||||||
else Def.task(oldCompile) | ||||||||||||||||||
}.value, | ||||||||||||||||||
// In some cases (I haven't been able to understand when/why, but this also happens for bgRunMain while | ||||||||||||||||||
|
@@ -505,8 +516,13 @@ object ScalafixPlugin extends AutoPlugin { | |||||||||||||||||
(config / scalafix / streams).value | ||||||||||||||||||
) | ||||||||||||||||||
} | ||||||||||||||||||
// sub-task of compile after which bytecode should not be modified | ||||||||||||||||||
task.dependsOn(config / manipulateBytecode) | ||||||||||||||||||
// we should not and cannot (circular dependency) trigger compilation | ||||||||||||||||||
// if this is an non-explicit invocation - we use the presence of a | ||||||||||||||||||
// flag rather than checking !scalafixInvoked, in order to trigger | ||||||||||||||||||
// compile in case scalafix is wrapped in another task that | ||||||||||||||||||
// scalafixInvoked does not know about | ||||||||||||||||||
if (shellArgs.extra.contains("--triggered")) task | ||||||||||||||||||
else task.dependsOn(config / compile) | ||||||||||||||||||
} else { | ||||||||||||||||||
Def.task { | ||||||||||||||||||
if (errors.length == 1) { | ||||||||||||||||||
|
@@ -583,7 +599,7 @@ object ScalafixPlugin extends AutoPlugin { | |||||||||||||||||
case "--stdout" => | ||||||||||||||||||
// --stdout cannot be cached as we don't capture the output to replay it | ||||||||||||||||||
throw StampingImpossible | ||||||||||||||||||
case "--tool-classpath" => | ||||||||||||||||||
case tcp if tcp.startsWith("--tool-classpath") => | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this PR exposed that a scripted was passing for the wrong reason: a scalafix invocation was failing because of a cache miss recompilation while it should have failed in scalafix itself sbt-scalafix/src/sbt-test/skip-windows/caching/test Lines 186 to 193 in 9ceb2c5
|
||||||||||||||||||
// custom tool classpaths might contain directories for which we would need to stamp all files, so | ||||||||||||||||||
// just disable caching for now to keep it simple and to be safe | ||||||||||||||||||
throw StampingImpossible | ||||||||||||||||||
|
@@ -743,16 +759,6 @@ object ScalafixPlugin extends AutoPlugin { | |||||||||||||||||
options.ignoredScalacOptions() ++ | ||||||||||||||||||
scalacOptionsToRelax.map(_.pattern()) | ||||||||||||||||||
) | ||||||||||||||||||
}, | ||||||||||||||||||
manipulateBytecode := { | ||||||||||||||||||
val analysis = manipulateBytecode.value | ||||||||||||||||||
if (!scalafixInvoked.value) analysis | ||||||||||||||||||
else { | ||||||||||||||||||
// prevent storage of the analysis with relaxed scalacOptions - despite not depending explicitly on compile, | ||||||||||||||||||
// it is being triggered for parent configs/projects through evaluation of dependencyClasspath (TrackAlways) | ||||||||||||||||||
// in the scope where scalafix is invoked | ||||||||||||||||||
analysis.withHasModified(false) | ||||||||||||||||||
Comment on lines
-751
to
-754
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this turned out to be unnecessary, since there is no scenario where a relaxed invocation would override a prior, regular one and invalidate the cache |
||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
) | ||||||||||||||||||
|
||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,8 @@ | ||
val V = _root_.scalafix.sbt.BuildInfo | ||
|
||
addCompilerPlugin(scalafixSemanticdb) | ||
scalacOptions ++= Seq("-Yrangepos") | ||
|
||
scalacOptions ++= Seq("-Yrangepos", "-Ywarn-unused") | ||
scalaVersion := V.scala212 | ||
scalacOptions ++= Seq( | ||
// generate errors on unused imports | ||
"-Xfatal-warnings", | ||
"-Ywarn-unused" | ||
) | ||
Compile / compile / scalacOptions ++= Seq( | ||
// generate errors on procedure syntax | ||
"-Wconf:cat=deprecation:e", | ||
|
@@ -38,11 +32,6 @@ TaskKey[Unit]("checkLastCompilationCached") := { | |
} | ||
} | ||
|
||
TaskKey[Unit]("checkZincAnalysisPresent") := { | ||
val isPresent = (Compile / previousCompile).value.analysis.isPresent() | ||
assert(isPresent, "zinc analysis not found") | ||
} | ||
Comment on lines
-41
to
-44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed this helper for whitebox testing since the blackbox one is enough |
||
|
||
// https://github.com/sbt/sbt/commit/dbb47b3ce822ff7ec25881dadd71a3b29e202273 | ||
// must be outside a macro to workaround "Illegal dynamic reference: Def" | ||
def streamScopedKey(scope: Scope) = Def.ScopedKey(scope, Keys.streams.key) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,45 @@ | ||
# we have 2 fatal warnings preventing compilation | ||
# when the build does not contain any flag relaxed on scalafix invocation | ||
# check that regular compilation benefits froma prior direct scalafix invocation ... | ||
-> scalafix --check RemoveUnused | ||
-> checkLastCompilationCached | ||
> compile | ||
> checkLastCompilationCached | ||
|
||
Comment on lines
+5
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. users with no fatal warnings calling scalafix before compile no longer have to compile everything twice |
||
# ... and indirect scalafix invocation | ||
> clean | ||
> test:scalafix --check RemoveUnused | ||
-> checkLastCompilationCached | ||
> compile | ||
> checkLastCompilationCached | ||
|
||
# we now have 2 fatal warnings preventing compilation | ||
> set scalacOptions ++= Seq("-Xfatal-warnings") | ||
-> compile | ||
|
||
# ensure that a semantic rule can be ran despite warnings, to fix one of the warning | ||
> scalafixAll RemoveUnused | ||
> scalafix RemoveUnused | ||
|
||
# but that the zinc analysis file has not been persisted, otherwise the run with relax scalacOptions would | ||
# cause the next compile to be full (non-incremental) no matter if there was a previous successful compilation | ||
-> checkZincAnalysisPresent | ||
# confirm that the rule was applied and that triggered compilation is cached once the updated file recompiled | ||
> scalafix --check RemoveUnused | ||
-> checkLastCompilationCached | ||
> scalafix --check RemoveUnused | ||
> checkLastCompilationCached | ||
|
||
Comment on lines
+23
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. successive |
||
# and that -Xfatal-warnings remains honored for regular compilation | ||
-> compile | ||
|
||
# confirm that compilation succeeds after fixing the last warning | ||
# confirm that regular compilation succeeds after fixing the last warning | ||
> scalafix ProcedureSyntax | ||
> compile | ||
> checkZincAnalysisPresent | ||
|
||
# check that there is no recompilation when running a semantic rule after a successful compilation | ||
-> checkLastCompilationCached | ||
> scalafixAll RemoveUnused | ||
# check that regular compilation is cached as usual | ||
> compile | ||
> checkLastCompilationCached | ||
|
||
# check that there is no recompilation when running a semantic rule after a regular compilation ... | ||
> scalafix RemoveUnused | ||
> checkLastCompilationCached | ||
|
||
# nor when compiling after running a semantic rule | ||
> compile | ||
> checkLastCompilationCached |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #411 (comment). I can't tell for sure, but I think that storing analysis files slightly changed the timing of concurrent executions of
compile
, causing that new transient failure.