Skip to content
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

fix usage of scalafix via another task #152

Merged
merged 2 commits into from
Jul 15, 2020

Conversation

bjaglin
Copy link
Collaborator

@bjaglin bjaglin commented Jul 15, 2020

Regression of 0cf5501: initial (cold) invocations to a task delegating to scalafix would result in scalafix being called before compilation (because scalafixRunExplicitly == false) causing false negatives.

This makes sure compilation is always triggered before scalafix (no matter how it's called), but depends on the final sub-tak of compile in a cycle-free way.

@@ -0,0 +1,14 @@
# check that missing rewrites are detected, with or without a warm compilation cache
> set scalafixOnCompile := false
-> lintAll
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on master

[info] [info] Non-compiled module 'compiler-bridge_2.12' for Scala 2.12.12. Compiling...
[info] [info] Running scalafix on 2 Scala sources
[error] /tmp/sbt_4021633e/wrapper/src/main/scala/example/Example.scala:3: error: not found: object imported
[error] import imported.Imported
[error]        ^
[info] [info]   Compilation completed in 14.904s.
[info] [warn] /tmp/sbt_4021633e/wrapper/src/main/scala/example/Example.scala:3:17: Unused import
[info] [warn] import imported.Imported
[info] [warn]                 ^
[info] [warn] one warning found
[info] [info] Done compiling.
[info] [success] Total time: 20 s, completed Jul 15, 2020, 11:52:59 AM
[error] x sbt-scalafix / wrapper 
[error]  Cause of test exception: {line 3}  Command succeeded but failure was expected

Copy link
Collaborator Author

@bjaglin bjaglin Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I investigated where the concurrent Non-compiled module 'compiler-bridge_2.12' for Scala 2.12.12. Compiling.../Done compiling was coming from as it looked like it could race with the bytecode consumption in scalafix. It turns out to be from Test / scalafix, so I added bjaglin@3a0a4c8 to strengthen the test.

Without the fix, the test logs now shows:

[info] [info] Running scalafix on 2 Scala sources
[error] /tmp/sbt_93ed664/wrapper/src/main/scala/example/Example.scala:3: error: not found: object imported
[error] import imported.Imported
[error]        ^
[info] [success] Total time: 5 s, completed Jul 15, 2020, 5:29:15 PM
[error] x sbt-scalafix / wrapper 
[error]  Cause of test exception: {line 3}  Command succeeded but failure was expected

Regression of 0cf5501: initial (cold) invocations to a task delegating
to scalafix would result in scalafix being called before compilation
(because scalafixRunExplicitly == false) causing false negatives.
@bjaglin bjaglin marked this pull request as ready for review July 15, 2020 13:41
@bjaglin bjaglin requested a review from olafurpg July 15, 2020 13:42
Through Test/scalafix & then Test/internalDependencyClasspath,
scalafixAll evaluates Compile/compile, which could make
Compile/scalafix run as expected sometimes before the fix in
68b7e82.
@bjaglin
Copy link
Collaborator Author

bjaglin commented Jul 15, 2020

(looks like GitHub is having another outage, and my second commit on https://github.com/bjaglin/sbt-scalafix/commits/task-wrappers is not visible here)

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see to see scalafixRunExplicitly go away!

if (scalafixRunExplicitly.value) task.dependsOn(compile.in(config))
else task
// sub-task of compile after which bytecode should not be modified
task.dependsOn(manipulateBytecode.in(config))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bjaglin bjaglin merged commit ee178da into scalacenter:master Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants