Support linking and running of Scala Native projects #457

Duhemm · 2018-04-30T05:19:14Z

I have an implementation that also supports testing of Scala Native projects; however it needs the test runner to be split out of sbt-scala-native (see scala-native/scala-native#1234).

Also missing from this PR: extracting the other options relevant only for Scala Native (GC to use, release vs debug mode, etc.)

This is already large enough for one PR, I think it'd be better to open another PR for the rest.

jvican · 2018-05-01T22:20:48Z

frontend/src/main/scala/bloop/cli/Commands.scala

@@ -156,4 +158,19 @@ object Commands {
      watch: Boolean = false,
      @Recurse cliOptions: CliOptions = CliOptions.default
  ) extends CompilingCommand
+
+  case class NativeLink(


I think we should only have one command called link that is both js and native compatible. We can make the inputs or the options be runtime specific and only expose a very common subset of flags that they both share, but ergonomic-wise I think it's important we only have one command called link.

jvican

I'm super excited to support this in Bloop.

After giving it some thought, I don't think that resolving the native bridge at the runtime is a good idea. We should be moving away from runtime dep resolution instead of locking ourselves in an unnecessary dependency in coursier that should only be used in test (to resolve the scala instance). I plan on removing the current resolution of test agents at some point in the future, and therefore I'd prefer another solution to the bridge.

I think a more solid approach to this problem (and also the future Scalajs compat) would be to store the Scala native jars in the bloop config file (in case the project supports it), and then create a new root (cached) classloader with those jars together with NativeBridge class file. We would obtain that class file as a runtime resource, and we would make sure that sbt compiles the native bridge and then dumps the class file in the resources directory of frontend.

I know it sounds like a pretty custom solution, but I'm afraid that if we merge this as is we'll go down the rabbit hole of dependency resolution, which I'd really like to avoid.

I just left some other comments. Overall the PR looks solid! I have a few more questions:

Shouldn't we also add a field in the bloop config file that takes potential native configuration from the host build (e.g. sbt), and then make sure that SbtBloop exports that info?
Shouldn't the native classloader be cached?

jvican · 2018-05-01T22:21:07Z

frontend/src/main/scala/bloop/cli/Commands.scala

+      @HelpMessage("The main class to link. Leave unset to let bloop select automatically.")
+      main: Option[String] = None,
+      @HelpMessage("Pick reporter to show compilation messages. By default, bloop's used.")
+      reporter: ReporterKind = BloopReporter,


Why do we need the reporter here?

It is needed because the link command may trigger a compilation pass.

jvican · 2018-05-01T22:21:37Z

frontend/src/main/scala/bloop/cli/Commands.scala

+      reporter: ReporterKind = BloopReporter,
+      @ExtraName("w")
+      @HelpMessage("If set, run the command whenever projects' source files change.")
+      watch: Boolean = false,


Do we really need watch here? If so, can we add tests for it?

Yes, at least for Scala.js --watch is desirable because after every source change a new JavaScript file would be assembled. sbt supports the same with ~fastOptJS.

jvican · 2018-05-01T22:22:33Z

frontend/src/main/scala/bloop/engine/Interpreter.scala

+              case Some(main) =>
+                Task {
+                  val out = ScalaNative.nativeLink(project, main, state.logger)
+                  state.logger.info(s"Produced $out")


Can we have a more descriptive log?

jvican · 2018-05-01T22:24:16Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNative.scala

+    // The Scala Native toolchain expects to receive the module class' name
+    val fullEntry = if (entry.endsWith("$")) entry else entry + "$"
+
+    AbsolutePath(nativeLinkMeth.invoke(null, project, fullEntry, logger).asInstanceOf[Path])


Can we protect us here from a potential null returned by invoke and only instantiate an absolute path if we're 100% sure the reflective execution didn't fail?

I think it is cleaner to pass the target path to nativeLink(). Then, we can ignore the result altogether.

jvican · 2018-05-01T22:26:21Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNative.scala

+          cwd: AbsolutePath,
+          main: String,
+          args: Array[String]): Task[State] = Task {
+    import scala.collection.JavaConverters.propertiesAsScalaMap


Could you rebase this on top of #449? Also, can we not reuse as much as possible the stock implementation of run? This seems to boilerplate-y 😄

jvican · 2018-05-02T09:18:57Z

Oh, and shouldn’t we also have a class file manager for every NIR file it’s out putted by the Scala Native plugin?

tindzk · 2018-05-21T17:03:34Z

@jvican I share your concerns about resolving the bridges at runtime. At the moment, bridges depend on the front-end and on the back-end JARs. I am creating a Docker image for Bloop with Scala.js support that can be easily used in a CI like Drone. With the current design, packaging the JARs in .bloop does not suffice. I also need to include .ivy2, so eventually end up with two JARs for the front-end and back-end each.

It would be appreciated if we could get rid of the Coursier dependency in Bloop as resolving dependencies should be ideally handled externally.

What do you think about making the bridges part of the front-end for now? We could modularise the code in a future pull request. As I understood, the reasoning behind modularising the bridges was that the dependencies on Scala Native and Scala.js should be opt-in, and that we need to support multiple versions like Scala.js 0.6 and 1.0. Is this correct?

jvican · 2018-06-07T13:44:15Z

config/src/main/scala/bloop/config/Config.scala

+  )
+
+  object NativeConfig {
+    private[bloop] val empty = NativeConfig(Array.empty,


Nitpick: can we move this to only one line like we do in Project.empty?

jvican · 2018-06-07T13:46:16Z

frontend/src/main/scala/bloop/engine/Interpreter.scala

+        Task(reportMissing(cmd.project :: Nil, state))
+      case Some(project) if project.platform != Platform.Native =>
+        Task {
+          state.logger.error("This command can only be called on a Scala Native project.")


When you add Scala.js support, can we improve this message and have it be like: "Expected $PROJECT_NAME to be either a Scala.js or Scala Native project"?

jvican · 2018-06-07T13:49:37Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNative.scala

+
+}
+
+class ScalaNative private (classLoader: ClassLoader) {


Can we move the class before the companion?

jvican · 2018-06-07T13:50:11Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNative.scala

+        Forker.run(cwd, cmd, state.logger, state.commonOptions).map { exitCode =>
+          val exitStatus = {
+            if (exitCode == Forker.EXIT_OK) ExitStatus.Ok
+            else ExitStatus.UnexpectedError


Can we use our own status code, like LinkingError and use it in all the places?

jvican · 2018-06-07T13:50:59Z

frontend/src/main/scala/bloop/exec/Forker.scala

+object Forker {
+
+  /** The code returned after a successful execution. */
+  final val EXIT_OK = 0


Make both of them private[Forker]?

That value is being used in other places.

jvican · 2018-06-07T13:54:59Z

config/src/main/scala/bloop/config/Config.scala

+      linkingOptions: Array[String],
+      compileOptions: Array[String],
+      targetTriple: String,
+      nativelib: Path,


nativeLib for consistency?

That's how it's called in Scala Native's build API

jvican · 2018-06-07T13:55:11Z

config/src/main/scala/bloop/config/Config.scala

+      toolchainClasspath: Array[Path],
+      gc: String,
+      clang: Path,
+      clangPP: Path,


What does clangPP mean?

Isn't it possible to have something like clang++? (Note the backticks.) I wonder if circe would do the right thing, I guess it will.

Personally, I prefer to keep it simple and name everything the way it is in Scala Native.

jvican · 2018-06-07T13:57:34Z

config/src/main/scala/bloop/config/Config.scala

+    }
+  }
+
+  case class NativeConfig(


It would be great if there was a scaladoc explaining each of the fields of this case class, as well as the options that they can take (e.g. gc). If this is too cumbersome, a link to some kind of Scala Native documentation would also be helpful

jvican

Looks great, good job @Duhemm 👍

My only high-level comment is that we should probably change how we model NativeConfig in the json file. This can be addressed in the follow-up PR integrating Scala.js because it will also touch on its design.

Some brainstorming: let's have a field platforms: List[Platform] in the config and have NativeConfig as a NativePlatform <: Platform. We could do the same for scalajs with JavascriptPlatform and JavaPlatform. JavaPlatform would also take the fields of Jvm. WDYT?

jvican · 2018-06-07T14:00:28Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNative.scala

+
+import monix.eval.Task
+
+object ScalaNative {


Can we call this something else that clarifies more the meaning of the class? ScalaNativeToolchain for example?

jvican · 2018-06-07T14:01:03Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNative.scala

+  def forProject(project: Project, logger: Logger): ScalaNative = {
+    project.nativeConfig match {
+      case None =>
+        ivyResolved(logger)


s/ivyResolved/resolveBridgeJars/g

jvican · 2018-06-07T14:03:01Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNative.scala

+  }
+
+  def direct(classpath: Array[AbsolutePath]): ScalaNative = {
+    instancesCache.computeIfAbsent(classpath,


Hoist up the creation of ScalaNative up to its own variable?

jvican · 2018-06-07T14:04:05Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNative.scala

+  }
+
+  def ivyResolved(logger: Logger): ScalaNative = synchronized {
+    if (_ivyResolved == null) {


What is the purpose of _ivyResolved in the grand scheme of things? Don't we already cache with instancesCache and synchronize on the object here?

This is only to completely bypass coursier (though I remember measuring it when we discussed about resolving the test agents, and it took only a few ms). I'm happy to get rid of it.

Just replied with another comment re caching.

jvican · 2018-06-07T14:45:20Z

frontend/src/main/scala/bloop/engine/tasks/ScalaNativeToolchain.scala

+  }
+
+  def resolveNativeToolchain(logger: Logger): ScalaNativeToolchain = {
+    val jars = bridgeJars(logger)


Last time I was touching ScalaInstance I noticed that the overhead of calling coursier is high, and skipping its invocation reduced starting up time significantly.

I think we should do the same we do with ScalaInstance here https://github.com/scalacenter/bloop/blob/topic/allow-only-java-projects/backend/src/main/scala/bloop/ScalaInstance.scala#L106-L107 so that we can share the resolution of the bridges across every project not defining the toolchainClasspath.

How high was it in your measurement? Resolving bloop-native-bridge_2.12 is pretty fast on my machine, but I may very well be benchmarking it wrong:

[info] Benchmark Mode Cnt Score Error Units [info] Bench.resolveNativeBridge sample 4056 24.671 ± 0.086 ms/op [info] Bench.resolveNativeBridge:resolveNativeBridge·p0.00 sample 22.315 ms/op [info] Bench.resolveNativeBridge:resolveNativeBridge·p0.50 sample 24.052 ms/op [info] Bench.resolveNativeBridge:resolveNativeBridge·p0.90 sample 27.197 ms/op [info] Bench.resolveNativeBridge:resolveNativeBridge·p0.95 sample 27.787 ms/op [info] Bench.resolveNativeBridge:resolveNativeBridge·p0.99 sample 29.014 ms/op [info] Bench.resolveNativeBridge:resolveNativeBridge·p0.999 sample 30.079 ms/op [info] Bench.resolveNativeBridge:resolveNativeBridge·p0.9999 sample 31.818 ms/op [info] Bench.resolveNativeBridge:resolveNativeBridge·p1.00 sample 31.818 ms/op

The resolution of bridges was shared between projects that don't define toolchainClasspath. Should I revert 27cd1a3 ?

jvican · 2018-06-07T14:51:16Z

config/src/main/scala/bloop/config/Config.scala

+
+  /**
+   * Configures how to start and use the Scala Native toolchain, if needed.
+   * See http://javadoc.io/doc/org.scala-native/tools_2.10/0.3.7 for the description of the fields.


This link points to the root of the scaladoc, would it be possible to link to the class containing all the fields? 😄

The extraction is done simply by checking whether the Scala Native or Scala.js plugin is enabled. If none of them is enabled, then the platform is assumed to beJVM. It could be interesting to use sbt-cross-project to check the platform, but it is possible to create a Scala Native or Scala.js project without using sbt-cross-project. The current solution has the advantage of working in either cases. In the maven plugin, the platform is always chosen to be JVM.

Forker used to allow running only a main class in a Java program. This commit refactors it, so that it can be used to run an arbitrary command. This will be useful to be able to run, for instance, programs compiled with Scala Native.

This project is a new component that has a dependency on Scala Native's toolchain. The idea is that we don't want Bloop to always depend on Scala Native, so we add this component that will be downloaded and loaded when needed.

The implementation is copy-pasted from the example of how to use the build API in Scala Native.

This task is equivalent to `nativeLink` in the Scala Native sbt plugin (it produces the runnable native binary).

This project will help us test Bloop with projects that cross-compile to Scala.js and Scala Native.

This field contains the classpath that should be loaded in order to find the native bridge for Bloop. If this field is not set, then the bridge is resolved using Coursier.

This replaces the previous `nativeClasspath`. `nativeConfig` contains all the configuration that should be required for a Scala Native project, including the classpath of the Scala Native toolchain. This configuration item is currently optional. If it is not specified, then the Scala Native toolchain will be resolved using Coursier and the default values will be used for configuring the toolchain. At the moment, our sbt and Maven plugin never populate this configuration.

This is necessary, because the optimizer will produce a different number of files depending on its configuration. The files produced by a previous run, with a different configuration, may be conflicting with a subsequent run.

This flag configures the optimization level of the produced code when an optimizer is present (that is, when the project is compiled with Scala.js or Scala Native). The flag accepts two values, `debug` (fast compilation, slower code), and `release` (slower compilation, faster code).

This allows us to completely bypass Coursier if the resolution has already been done previously.

jvican

Great job, looking forward to the Scalajs integration 🎉

Duhemm added enhancement task / compile task / run configuration format Assigned to any ticket or pull request that will change the configuration file format. Scala Native labels Apr 30, 2018

Duhemm mentioned this pull request Apr 30, 2018

Support Scala Native #458

Closed

7 tasks

Duhemm force-pushed the topic/support-scala-native-runonly branch from aed7707 to 7064c70 Compare April 30, 2018 12:23

jvican reviewed May 1, 2018

View reviewed changes

tindzk mentioned this pull request May 13, 2018

Add Scala.js bridge #485

Closed

2 tasks

Duhemm force-pushed the topic/support-scala-native-runonly branch 9 times, most recently from df7a823 to ecb27bb Compare June 7, 2018 13:55

jvican reviewed Jun 7, 2018

View reviewed changes

Duhemm force-pushed the topic/support-scala-native-runonly branch from ecb27bb to f99cd83 Compare June 7, 2018 14:34

jvican reviewed Jun 7, 2018

View reviewed changes

Duhemm added 3 commits June 8, 2018 07:23

Extract platform from config files

0444633

Add scripted tests for platform extraction

1ff51bd

Duhemm added 21 commits June 8, 2018 07:23

Support running an arbitrary process in Forker

db1d00a

Forker used to allow running only a main class in a Java program. This commit refactors it, so that it can be used to run an arbitrary command. This will be useful to be able to run, for instance, programs compiled with Scala Native.

Add nativeBridge to interface with Scala Native

2b0c2dc

This project is a new component that has a dependency on Scala Native's toolchain. The idea is that we don't want Bloop to always depend on Scala Native, so we add this component that will be downloaded and loaded when needed.

Add implementation of NativeBridge

0a16ab4

The implementation is copy-pasted from the example of how to use the build API in Scala Native.

Pass explicit logger to Scala Native toolchain

501cb8a

Pass the module class' name to Scala Native

1cec268

Add native-link task to Bloop

44d12af

This task is equivalent to `nativeLink` in the Scala Native sbt plugin (it produces the runnable native binary).

Support running of Scala Native projects

6715481

Address review comments

c03fb87

Add cross-platform to integration projects

599faff

This project will help us test Bloop with projects that cross-compile to Scala.js and Scala Native.

Add nativeClasspath to Config

9b1ff88

This field contains the classpath that should be loaded in order to find the native bridge for Bloop. If this field is not set, then the bridge is resolved using Coursier.

Add first tests for Scala Native integration

68e900a

Add nativeBridge to release commands

7c7fccf

Native bridge: Delete workdir before linking

da2a76e

This is necessary, because the optimizer will produce a different number of files depending on its configuration. The files produced by a previous run, with a different configuration, may be conflicting with a subsequent run.

Lowercase platform names

736aad6

Address review feedback

48796ba

Rename ScalaNative to ScalaNativeToolchain

1cffb38

Simplify caching of ScalaNativeToolchain instances

e8b0b4f

Add LinkingError exit code

68aca53

Cache Scala Native toolchain resolved by Coursier

b7d1f0b

This allows us to completely bypass Coursier if the resolution has already been done previously.

Duhemm force-pushed the topic/support-scala-native-runonly branch from f99cd83 to b7d1f0b Compare June 8, 2018 05:23

jvican approved these changes Jun 8, 2018

View reviewed changes

jvican merged commit 180254c into master Jun 8, 2018

tgodzik deleted the topic/support-scala-native-runonly branch September 7, 2021 16:10

Support linking and running of Scala Native projects #457

Support linking and running of Scala Native projects #457

Conversation

Duhemm commented Apr 30, 2018

Choose a reason for hiding this comment

jvican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented May 2, 2018

tindzk commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican left a comment

Choose a reason for hiding this comment

jvican Jun 7, 2018 •

edited

Loading