Cleanup rules, esp. executables, to be more consistent #171
Conversation
In trying to get these rules to work in other build environments (with different locations and ways for building scalac, javac, etc.) found that various constructs were not being applied properly. Also, in general, the launchers for scala_test, scala_binary, and scala_repl were not resilient to the various permutations of places that Bazel will place your runfiles. In particular, a scala_binary could not reliably be used in a genrule or shell test. Thus... The biggest change is unifying the launcher for all three of the executable rules (see _write_launcher). This borrows from the form in the java_skylark rules to inspect itself and more reliably find its runfiles. As a bonus, now all rules set CLASSPATH now. Also, importantly, the binary rules (whose launcher is actually a shell script) were not setting up their runfiles correctly (this is kind of partly evidenced by them directly depending on _java and _jdk since _java runfiles should be including a _jdk). So, the rules now define two helper functions for getting runfiles and then use them. At the moment, was only necessary to grab the runfiles of _java, since rjars already was a transitive list. Added tests that the scala_binary outputs could be run in a genrule and test. I am still slightly concerned of edge cases (as the nesting gets even deeper, like if a genrule has an executable that itself has a scala_binary as a runfile). Other changes are: * For the implicit deps, many of them were marked as single_file when they did not need to be (in particular, any executable). That marking has been removed and the various associated uses of file were replaced by executable. * I am fairly certain that ctx.action already pulls in runfiles of all inputs, so for the scalac action, amended its input list to pull in just what is relevant (and prefer pulling in executable over files when it can). * scala_repl no longer relies on bin/scala but instead just invokes it directly from scalac. To do this, also had yo add _scalacompiler to repl rjars
|
Can one of the admins verify this patch? |
sdtwigg
changed the title
|
I did find this in the Bazel sources right after sending out this PR: A key difference is that it doesn't export JAVA_RUNFILES (which will affect the case when a scala_binary calls a scala_binary or java_binary). Also, has a significant amount of additions for handling the OS X and Windows environment. Actually, with the release of all the pieces for the java sandwich, it might be simpler in the long run to just port to using the sandwich and then just have scala_test and scala_binary be Skylark macros? (Although, then the scala_binary invocation will show up as two rules: one to compile the underlying library and another to build the binary.) |
|
This is good work. I really appreciate it. One issue I'm concerned with is removing the scala shell script. For reference here it is: note on line 26 is does some stty work. Without this, you have issues with using the repl. Scalding has to do something similar: Java does not have a repl (yet, but will in java9?), so I think we still need custom code for that. Looking in detail at the PR now. |
| @@ -84,14 +95,16 @@ def _build_nosrc_jar(ctx, buildijar): | |||
| cp_resources=cp_resources, | |||
| out=ctx.outputs.jar.path, | |||
| manifest=ctx.outputs.manifest.path, | |||
| java=ctx.file._java.path, | |||
| java=ctx.executable._java.path, | |||
There was a problem hiding this comment.
I didn't know these triggered different side effects. Good to know.
There was a problem hiding this comment.
This shouldn't change the local operation here. However, I needed this so that whatever rule is emitting the _java label was not restricted to emitting a single file. (For example, any binary output by java_binary will still be executable and resolve under ctx.executable but have multiple files and thus not resolve under ctx.file).
I also think it is a little clearer since we are using java as an executable in the actual command.
| @@ -384,6 +395,9 @@ def _lib(ctx, non_macro_lib): | |||
| transitive_compile_exports=texp.compiletime, | |||
| transitive_runtime_exports=texp.runtime | |||
| ) | |||
|
|
|||
| # Note that rjars already transitive so don't really | |||
There was a problem hiding this comment.
should we collect just the direct runtime jars and not transitive here? Is there any win to not manually accumulating the rjars ourselves? I didn't know about transitive runfiles (maybe they didn't exist when this was written?)
There was a problem hiding this comment.
Just so there is no confusion, adding to transitive_files doesn't seem to invoke any sort of transitive searching. (In all my tests, you could just .to_list() whatever was being added to runfiles.transitive_files and add that to the runfiles.files instead for no difference).
I am fairly certain transitive_files is just for slightly improved performance over files (since it takes a depset). I haven't yet had time to really dig into the bazel implementation to verify this. My current best guess is that transitive_files is resolved and flattened into the runfiles lazily, rather thus can skip that step if the file doesn't actually show up as a runfile. In this case, we already had to linearize the rjars to pass them along in the provider so no cost.
I'm not too worried about making these runfiles perfect because I imagine we will soon move over to using java_common.create_provider anyway and then just setup runfiles and providers using that.
scala/scala.bzl
Outdated
|
|
||
| jvm_flags = " ".join( | ||
| ["-Dscala.usejavacp=true"] + | ||
| ["-J" + flag for flag in ctx.attr.jvm_flags] |
There was a problem hiding this comment.
I don't think we need -J if you are not passing to the scala script. We should have a way to test flags.
scala/scala.bzl
Outdated
| ctx = ctx, | ||
| rjars = rjars, | ||
| main_class = ctx.attr.main_class, | ||
| jvm_flags = "", |
There was a problem hiding this comment.
seems like we should want to support jvm_flags on scala_binary and now it is easy. Can we do that? They should be in the common attributes: " ".join(ctx.attr.jvm_flags) I think.
scala/scala.bzl
Outdated
| ["-J" + flag for flag in ctx.attr.jvm_flags] | ||
| ) | ||
| args = " ".join(ctx.attr.scalacopts) | ||
| _write_launcher( |
There was a problem hiding this comment.
I'm concerned this REPL is not going to set the stty issues correctly:
https://gist.github.com/johnynek/6c702b794a6737e480b2eded529b1509#file-scala-sh-L26
Maybe we can pass an additional prefix and suffix to the script where we can add this logic? I would like to not have to use the scala script and just consume the jars.
There was a problem hiding this comment.
Good point. The use of prefix and suffix sounds reasonable. Will try to get this cleaned up tonight.
scala/scala.bzl
Outdated
| ctx = ctx, | ||
| rjars = rjars, | ||
| main_class = ctx.attr.main_class, | ||
| jvm_flags = "", |
There was a problem hiding this comment.
can't we support jvm_flags here too?
All binaries (repl, test, and binary) will now pass the JVM flags along. Also, no longer unnecessarily prepending -J to those flags. The scala_repl launcher will now save and restore stty like is done in the bin/scala script. In implementation, this was done by adding two new parameters to _write_launcher. Restore deprecation warning for suites argument in scala_test
|
Propagated jvm_flags to all 3. Cleaned stty in scala_repl. |
|
Thanks! |
|
test this please |
In trying to get these rules to work in other build environments
(with different locations and ways for building scalac, javac, etc.)
found that various bazel constructs were not being applied properly.
This commit tries to clean some of that up and make things slightly
more consistent and reliable.
Also, in general, the launchers for scala_test, scala_binary, and
scala_repl were not resilient to the various permutations of places that
Bazel will place your runfiles. In particular, a scala_binary could not
reliably be used in a genrule or shell test. Thus...
The biggest change is unifying the launcher for all three of the
executable rules (see _write_launcher). This borrows from the form in
the java_skylark rules to inspect itself and more reliably find its
runfiles. As a bonus, now all rules set CLASSPATH now.
Also, importantly, the binary rules (whose launcher is actually a
shell script) were not setting up their runfiles correctly (this is kind
of partly evidenced by them directly depending on _java and _jdk since
_java runfiles should be including a _jdk for you). So, the rules now define two
helper functions for getting runfiles and then use them. At the moment,
was only necessary to grab the runfiles of _java, since rjars already
was a transitive list.
Added tests that the scala_binary outputs could be run in a genrule and
test. I am still slightly concerned of edge cases (as the nesting gets
even deeper, like if a genrule has an executable that itself has a
scala_binary as a runfile).
Other changes are:
For the implicit deps, many of them were marked as single_file when they
did not need to be (in particular, any executable). That marking has
been removed and the various associated uses of file were replaced by
executable.
I am fairly certain that ctx.action already pulls in runfiles of all
inputs, so for the scalac action, amended its input list to pull in just
what is relevant (and prefer pulling in executable over files when it
can).
scala_repl no longer relies on bin/scala but instead just invokes it
directly from scalac. To do this, also had yo add _scalacompiler to repl
rjars