Add --link flag to compile to allow linking through BSP #2535
Conversation
|
@tgodzik I'm not quite sure where/how I can write a test for this. I looked at BspCompileSpec, but I'm not quite sure how I can pass in the compileArgs there. I also want to further investigate some bugs with the JS linker config as it does not seem to respect everything that is set from bloop-config, but maybe that should be it's own PR? |
Before this commit linking could only be done through the run command with BSP. Because the BSP protocol does describe a linking command (as it is often a part of the compile step) the flag --link has been added to the compile command. This is useful for those kinds of projects that need the linker to be run to produce source code for further toolchains, e.g. a ScalaJS project might often use @JSExport and then run the code from some other JS entrypoint.
|
this should also fix: #1304 |
|
The JS-bridge defines this: val moduleInitializers = mainClass match {
case Some(mainClass) if runMain =>
logger.debug(s"Setting up main module initializers for $project")
List(ModuleInitializer.mainMethodWithArgs(mainClass, "main"))
case _ =>
if (runMain) {
logger.debug(s"Setting up no module initializers, commonjs module detected $project")
Nil // If run is disabled, it's a commonjs module and we link with exports
} else {
// There is no main class, install the test module initializers
logger.debug(s"Setting up test module initializers for $project")
List(
ModuleInitializer.mainMethod(
TestAdapterInitializer.ModuleClassName,
TestAdapterInitializer.MainMethodName
)
)
}
}I disagree here. Not running main is indicative of JSExport and the likes I think, not that we want to link with tests (I don't think this is very common). I propose here that we instead add a test flag for the test case and add that same flag to the Commands.Test (it can also be an additional flag in the compileArgs). |
| (state, result) | ||
| } | ||
| case Jvm(_, _, _, _, _, _) => | ||
| compileProjects(userProjects :: Nil, state, compileArgs, originId, logger) |
There was a problem hiding this comment.
Alternatively here we can do the same as in the interpreter when it hits a JVM project on link. I'm not sure if there is much difference.
This should be similar to 32dbfb1#diff-9458dec34378d1535538ca59de86e441bcef70b5739a0cc484281ac8ed1ef1aaR695
Would probably be easier to review, but I can also ask for help reviewing |
I haven't worked in that part too much, but I think that might have been a simplification since Bloop wasn't used for publishing anything |
- Added better differentiation of what is a test task - Makes sure that compile is run before link (compiles are properly cached). - Linking now properly works for projects that use JSExport instead of a main class
| case Success(linkRunsSeq) => | ||
| linkRunsSeq | ||
| .map(_._2) | ||
| .foldLeft(Option.empty[CompileResult])((_, res) => // TODO propogate stuff better here |
There was a problem hiding this comment.
This logic is somewhat lifted from the test function, but it doesn't really work that well here. I'm not entirely sure what the best strategy is for combining the compileresults (actually linker results)
|
So far I have tested that the changes in this PR allows for linking using JSExport and with a main method without having to call run. This is also tested through BSP in relation to this PR: oyvindberg/bleep#445. I have also added support for the new minification in ScalaJS which is now set to true for release esmodule builds. We can add an option for this in bloop-config so that users can override this setting if they want. Further TODOs for this PR:
@tgodzik anything else you think should be done? |
Nothing comes to mind, but maybe @sjrd will be able to take a quick look here? |
+ Ran formatter
Removes the dependency on frontend in jsbridge to avoid circular dependency when publishing bridge before running tests in frontend. The only reason jsbridge dependend on frontend was for logging the project name in jsbridge1, for jsbridge06 only the project name was needed for setting the output dir. frontend was also needed for running the tests for the bridge. Moving the bride test to it's own module let's us keep the dependecy graph neat and tidy.
…c as ScalaJS This uses the native buildTarget to check if we have an application or a dynamic library or a static library. If it is not an application we do not set the main class. Also inclued are changes to the frontend to allow for linking without a specified main class
|
@tgodzik this is now blocked by scalacenter/bloop-config#102 |
Running the release here https://github.com/scalacenter/bloop-config/actions/runs/12394117349 |
When linking a JVM project the link task now simply noops. This lets clients send all build projects to the link task and the task will only link where possible. This way the client does not need to be explicit when linking (although still possible). Errors when linking are now accumulated and error logged. Bsp returns an error code when encountering linking errors.
There was a problem hiding this comment.
Moving the these tests to a different module lets us remove the frontend dependency from the bridges. This allows us to create working tests for bsp in the bridges. However it breaks the ToolchainSpecs because the loading the bridge toolchain with the classloader no longer works.
I am no guru on these classloader issues. This is the part of the test that no longer works when separating the modules.
val state0: State = {
def setUpScalajs(p: Project): Project = {
val platform = p.platform match {
case jsPlatform: Platform.Js =>
jsPlatform.copy(toolchain = Some(ScalaJsToolchain.apply(this.getClass.getClassLoader)))
case _ => p.platform
}
p.copy(platform = platform)
}
val configDir = TestUtil.getBloopConfigDir("cross-test-build-scalajs-1.x")
val logger = bloop.logging.BloopLogger.default(configDir.toString())
TestUtil.loadTestProject(configDir, logger, true, _.map(setUpScalajs))
}@tgodzik do you have any neat trick to make this work?
There was a problem hiding this comment.
We could create a UriClassloader with the bridge in it?
There was a problem hiding this comment.
Actually I think the solution here is just to move these tests into frontend
|
Chages introduced in PR:
Overall This should bring bloop much closer to sbt/mill and the likes with regards to linking capabilities. There is probably still more work we can do to better the client capabilities when it comes to passing configuration, but I think we can add that later. Hopefully this PR will let us see more linking through bloop so we can discover possible bugs etc. I'm moving this from draft to ready, but we are still blocked on bloop-config. |
|
One thing I forgot to add is that I have not really done much with the scalajs bridge for 0.6. I have not added tests, or decoupled it from frontend like the others. I highly doubt there are many projects using bloop for such an old version. It should work as it did before however. |
This lets the client override the mode without making changes to the model. This is more in line with how fullLink/fastLink works in sbt. Using flags through BSP for this lets the client override the mode without having to change the actual build. This is very useful for this task.
- Logs warning when passing both --release and --debug arguments when linking through BSP compile. When both arguments are passed we simply ignore them and link with whatever liker mode is defined through bloop-config - fixup in build.sbt: remove commented out code
Before this commit linking could only be done through the run command with BSP. Because the BSP protocol does describe a linking command (as it is often a part of the compile step) the flag --link has been added to the compile command. This is useful for those kinds of projects that need the linker to be run to produce source code for further toolchains, e.g. a ScalaJS project might often use @JSExport and then run the code from some other JS entrypoint.