-
Notifications
You must be signed in to change notification settings - Fork 125
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
Move Scala Native linking to runtime fetched scala-native-cli process #449
Move Scala Native linking to runtime fetched scala-native-cli process #449
Conversation
a97ea52
to
311ddf6
Compare
I would like to confirm the way we handle reporting with the new Scala Native building structure. Previously when building SN programs scala-cli had a direct access to a SN Logger object, similarly as with JVM and JS. When running scala-native-cli (nativeTools cli api) as a separate process this is of course impossible. For now, scala-native-cli simply uses a default SN logger, with added options for individually filtering out log levels (debug (on stderr), info (on stdout), warn (on stdout), error (on stderr)). Scala-cli verbosity levels are mapped to that filtered logger. SN-cli is then run with IO inherited by scala-cli - this means we really have no direct access to errors thrown by SN (although they are being shown of course) and everything is reported with tags (like [info]...). This is the kind of thing that is unimportant in the short run, nor would I personally consider problematic in any way, but if we don't confirm it now I may not be able to change that later, after sn-cli will be published. Are those changes ok? EDIT 21.12.21: I forgot we always display tags with logs. It should be easy to just catch stdout/stderr and control output manually |
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.
Nice! I only left NIT comments.
The CI errors may be legit though… Feel free to add a commit replacing all the maybeBuild.toOption.get
in the tests by maybeBuild.fold(throw _, identity)
, so that the underlying errors aren't trapped.
modules/build/src/main/scala/scala/build/internal/NativeBuilderHelper.scala
Outdated
Show resolved
Hide resolved
modules/build/src/main/scala/scala/build/options/ScalaNativeOptions.scala
Outdated
Show resolved
Hide resolved
modules/build/src/test/scala/scala/build/tests/NativeBuilderHelperTests.scala
Outdated
Show resolved
Hide resolved
modules/build/src/main/scala/scala/build/errors/ScalaNativeBuildError.scala
Outdated
Show resolved
Hide resolved
@jchyb The logging stuff should be fine for now… I guess we can always tweak the Scala Native CLI itself later, to change how they're printed? |
@alexarchambault Thank you for the review! About the logging, we are planning to release one scala-native-cli per SN version (so now we are about to release 0.4.0, 0.4.1 and 0.4.2 and finalizing things), I think we can change some things for later versions, but we probably are not going to revisit the 0.4.x ones (then again, I imagine dropping support for older SN versions is not going to be a problem from the scala-cli side). |
684e38a
to
d84e0c9
Compare
This way we can freely change the SN version on scala-cli runtime. Logging was moved to the default scala native logger with log levels being filtered out (on scala-native-cli side) depending on verbosity set. Scala Native caching was also adjusted to facilitate incorrect options being passed to scala-native-cli.
3496468
to
4058d78
Compare
4058d78
to
f63df2b
Compare
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.
Apart from my (minor) question, LGTM!
That said, maybe for a later PR, I wish the discover stuff could be handled by scala-native-cli itself… In Scala CLI, we use "discovered" things in two places:
- to pass options to scala-native-cli
- to write the native config in bloop config files
Bloop actually accepts "empty" values, and can discover things on its own. If scala-native-cli handled discovery, we could get rid of it in Scala CLI (and drop the scala-native-tools dependency). Only --native-compile-defaults
and --native-linking-defaults
might be a problem (as these can't be put in a bloop config file), but we might be able to live without that (this part of the bloop config is only used from Metals, when it runs Scala CLI things via BSP, and ultimately, we'd like to handle this ourselves rather than delegating it to Bloop).
This is a PR meant to showcase changes to Scala Native linking before they have to be finalized so that no grave mistakes will be made on the scala-native-cli side. The most interesting part is in buildNative in Package.scala, where we run the linking process. Other changes concern redesigning Scala Native logging (we move it to scala-native-cli) and adjusted caching to accommodate the new structure.