-
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
Conversation
65df750
to
d63114b
Compare
// 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) |
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.
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
@@ -583,7 +585,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 comment
The 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
# files should be re-checked if a custom tool classpath is used (even if the rule is the same) | |
> set scalafixConfig := None | |
$ mkdir src/main/scala | |
$ copy-file files/Valid.scala src/main/scala/Valid.scala | |
> scalafix --tool-classpath=target/scala-2.12/classes RemoveUnused | |
$ exec chmod 000 src/main/scala/Valid.scala | |
-> scalafix --tool-classpath=target/scala-2.12/classes RemoveUnused | |
$ delete src/main/scala |
transient failure in inconfig scripted
|
@@ -56,7 +56,7 @@ scriptedSbt := { | |||
if (jdk >= 21) | |||
"1.9.0" // first release that supports JDK21 | |||
else | |||
(pluginCrossBuild / sbtVersion).value | |||
"1.3.3" // get https://github.com/sbt/sbt/issues/1673 to avoid race conditions |
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.
ccdab2b
to
67ce03d
Compare
> 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.
users with no fatal warnings calling scalafix before compile no longer have to compile everything twice
> scalafix --check RemoveUnused | ||
-> checkLastCompilationCached | ||
> scalafix --check RemoveUnused | ||
> 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.
successive scalafix
invocations on a cold environment (where no compile
was run successfully) now benefit from incremental compilation, no matter which flags are used in the build
TaskKey[Unit]("checkZincAnalysisPresent") := { | ||
val isPresent = (Compile / previousCompile).value.analysis.isPresent() | ||
assert(isPresent, "zinc analysis not found") | ||
} |
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.
I removed this helper for whitebox testing since the blackbox one is enough
The Zinc analysis is now saved on compilations triggered by scalafix invocations. This allows subsequent scalafix invocations not to re-trigger compilations, and subsequent regular compilations to be either incremental (in case the build contains `-Xfatal-warnings` or alike) or a full cache hit otherwise.
Follow-up of https://twitter.com/channingwalton/status/1778119590486118908 which puts the finger on a behavior that can be very irritating in large builds
This revisits #152, leveraging
scalafixInvoked
(which has the same semantics as thescalafixRunExplicitly
mentioned in the old PR, and didn't cause any issue for 3 years) in order to trigger a regularcompile
fromscalafix
and thus store zinc analysis to benefit later compilations.