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 |
| } | ||
| (linkState, result) | ||
| } | ||
| case Jvm(_, _, _, _, _, _) => ??? |
There was a problem hiding this comment.
@tgodzik Should we error here or just do a NoOp? if we do a NoOp a client can request linking all projects and only the ones that can link will be linked and trying to link JVM doesn't stop the linking. If we error the client has to be explicit and only try to link targets that can be linked.
There was a problem hiding this comment.
We should probably just compile normally in this case and ignore link option. A warning could be printed
Running the release here https://github.com/scalacenter/bloop-config/actions/runs/12394117349 |
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.