-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Prevent compilation during bloop config generation #1352
Prevent compilation during bloop config generation #1352
Conversation
@@ -195,81 +195,6 @@ class BloopImpl(ev: () => Evaluator, wd: Path) extends ExternalModule { outer => | |||
case _ => T.task(None) | |||
} | |||
|
|||
//////////////////////////////////////////////////////////////////////////// |
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.
moved below to prevent a forward-reference.
), | ||
mainClass = module.mainClass(), | ||
runtimeConfig = None, | ||
classpath = Some(classpath().map(_.toNIO).toList), |
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 is the only line that changed.
It would be good to understand what bloop uses this for
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.
Isn't that just classpath for compilation?
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.
Nope, the classpath for compilation is there
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 mean, yeah, the value that I'm assigning to the config (in this PR) is the the same as the one that's used for compilation, but here I'm asking about the meaning of that particular configuration element.
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.
Wait, that is in platform, let me check it.
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.
So this was added as a separate configuration for running on JVM. More info in the commit scalacenter/bloop@8898a93
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.
Right so according to the comment it could be left empty. Right ?
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.
It will use the main classpath if it's missing. Looks like it.
Bloop will fallback to the value present in the Project object.
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.
Looks good to me. Thanks!
Compilation of a module and its upstreams should be avoided at all cost when generating bloop configuration.
#1086 adds a reference to a task that depends on
compile
. This PR prevents that from happening, and adds a test to ensure that compilation isn't triggered byBloop/install
@tgodzik
@lolgab