Add _jdk dependency to binary runfiles and scalac action

[sdtwigg]

The default @local_jdk just imports _java and _javac as raw binaries
without establishing a dependency on/runfiles of _jdk. Thus, all uses of
those should manually add _jdk as a dependency.

In the scalac execution ation that compiles scala, also added _jdk in
case _scalac ever was replaced by a binary that wasn't a java_binary.

Haven't been able to test locally in a hermetic system with the default local_jdk so should wait until CI green before integrating....

[bazel-io]

Can one of the admins verify this patch?

[ittaiz]

Actually we probably need Oscar to say: test this please

…

On Tue, 28 Mar 2017 at 8:04 bazel.io machine account < ***@***.***> wrote: Can one of the admins verify this patch? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#172 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFx3hrid9846AZRS9ylLq0OYM7kBFks5rqJTugaJpZM4MrJH5> .

[ittaiz]

@johnynek @damienmg can one of you please trigger this for ci.bazel.io?
It seems travis doesn't provide confidence since it doesn't run the build sandboxed

[sdtwigg]

test this please

[damienmg]

Test this please.

[johnynek]

is this a bug in bazel? Why do we need to explicitly add this dep? cc @damienmg

[johnynek]

can we remove this comment now?

[johnynek]

and remove this one?

[sdtwigg]

Reverted that part of the change. Amended comment.

My fear is that transitive_deps to ijar COULD break code (although technically code that was making incorrect assumptions anyway). However, I was hoping for this PR to strictly decrease number of breakages.

[sdtwigg]

retest this please

[damienmg]

Not sure if that's a bug but according to @sdtwigg we don't do it internally so I would say we should just fix the dependencies in Bazel.

[ittaiz]

@sdtwigg why is the last commit needed? ci.bazel.io was green before it, wasn't it?

[damienmg]

According to the screen in our office, ci is red on darwin at HEAD: http://ci.bazel.io/job/rules_scala/684/

[sdtwigg]

It was not. There was a ctx.action in _build_nosrc_jar that was also invoking _java. The original commit was specifically targeted at fixing up the binary runfiles (and the scalac inputs because I had thought of it).

The second commit was after I realized it was still failing and did an 'audit' (ctrl-f for ctx.action and ctx.runfiles) for what all needed a _jdk.

The third commit was to undo the change to using ijar from java provider. (That was an experimental change that had snuck in on accident since I had done some experiments in my personal tree on switching to java_common stuff). That change did seem to work but I am nervous about using it without more extensive testing.

Finally, the fourth is just the first three all rebased together so the merge commit message makes sense.

[ittaiz]

@sdtwigg I understand. Are there changes which aren't protected by a test? The reason this troubles me is since it can break without us noticing. I understand the reason for the scalac case future-proofing and that has a comment. Any other cases? And do they have a big red flag comment on them?

[sdtwigg]

Are you referring to potential future changes that silently break or changes from #171 that might still be wrong?

Regardless....
Over the weekend, I was able to spend some time actually unpacking how ctx.action found its runfiles as well as what each argument in ctx.runfiles actually meant/where to find runfiles associated with targets. Quick summary of findings:

• ctx.action will look through all of its inputs and will only add runfiles to those inputs that are coming from attributes marked as executable=True. (This is why I suggest using ctx.executable._whatever in a ctx.action, to ensure that either the action will pickup runfiles or error if the target gets unmarked as an executable).
• ctx.runfiles files and transitive_files both just add runfiles to the same list. transitive_files is just a 'lazier' flattening (a depset) and so is slightly more efficient way to pass in transitive runfiles from dependencies. Notw that ctx.runfiles itself does not transitive searching (excluding collect_data and collect_default, which would only search deps, srcs, and data attributes anyway).
• ctx.attr.*.data_runfiles|default_runfiles should have runfiles for 'any labe'l attr (not extensively tested but bazel code seems to confirm), regardless of whether it was marked as executable or not or was created by a rule that returned a runfiles 'provider' . (In the latter case, would just get an empty depset).

Really, _java and _javac threw me a bit for a loop since they are executable but don't actually attach their needed runfiles. (_jar and _jar_bin should be fine since they are the deploy_jar and binary-via-launcher from a java_binary rule, respectively. Thus, they should both setup runfiles, including a JDK.)

[ittaiz]

Concerned about

potential future changes that silently break

[ittaiz]

To be more accurate- about potential future changes that silently break in rules_scala build but loudly break for users which, for some reason, depend on this behavior.

[johnynek]

Thanks for the fix @sdtwigg

[johnynek]

I think it is good not to break users, BUT if we find bugs people don't need to upgrade. They can stay on an old buggy version if they like. I think we should think twice before leaving bugs due to perceived compatibility reasons at this stage (how many users do we really have?)

…

On Mon, Mar 27, 2017 at 23:52 Ittai Zeidman ***@***.***> wrote: To be more accurate- about potential future changes that silently break in rules_scala build but loudly break for users which, for some reason, depend on this behavior. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#172 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdqVhyzJwCAnoetxoLePBhFfSSn7Lks5rqNhHgaJpZM4MrJH5> .

[ittaiz]

I might have been unclear. I didn't mean not to fix bugs. I meant that if we fix anything we should have a test for it so that if we want to later *refactor* for whatever reason we'll know we broke the feature and can be intentional about it. One obvious way is release notes. I think implicit context is problematic since it's forgotten. That's why I asked Stephen if there are missing tests. Hope I've made myself more clear, Ittai On Wed, 29 Mar 2017 at 10:19 P. Oscar Boykin <notifications@github.com> wrote:

…

I think it is good not to break users, BUT if we find bugs people don't need to upgrade. They can stay on an old buggy version if they like. I think we should think twice before leaving bugs due to perceived compatibility reasons at this stage (how many users do we really have?) On Mon, Mar 27, 2017 at 23:52 Ittai Zeidman ***@***.***> wrote: > To be more accurate- about potential future changes that silently break in > rules_scala build but loudly break for users which, for some reason, depend > on this behavior. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > < #172 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAEJdqVhyzJwCAnoetxoLePBhFfSSn7Lks5rqNhHgaJpZM4MrJH5 > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#172 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFxnxFLIrHMjPfiNeiG3gDzUvkmyGks5rqgYQgaJpZM4MrJH5> .

[sdtwigg]

Nothing immediately comes to mind; however, Bazel/Linux sandboxing isn't my area of expertise.

It is odd that only Darwin/macOS was so sensitive to sandboxing and neither of the Linux-based CI targets. I did test these rules locally with bazel 0.4.5 on my workstation, completely isolated from our regular build environment, although unfortunately it clearly wasn't completely sandboxed. Then, when I tested it in our regular build environment (which is substantially better at ensuring sandboxing), as Damien mentioned, there were a few subtle differences in how the local_jdk equivalent is actually setup causing me not to notice the issue. Fortunately, I should be more aware about checking for this in the future (and added the comments for others who might be similarly tempted to remove those inputs).

That said, I'm hoping that the reliance on knowing that java (and probably javac) has an undeclared dependency on the a jdk is temporary and that the local_jdk workspace is changed to be more honest about setting up runfiles for its executables.

I did try to add tests in a wider range of test scenarios in #171. For example, running a scala_binary in a test or genrule simply did not work for me before. I was also hoping to do some tests, eventually, with a scala_binary calling a java_binary or scala_binary given to it as a data dependency or as a command-line arg in a genrule. This would hopefully expose if exporting JAVA_RUNFILES was good or bad. (Although, would need to start with tests using just java_binary to see what the 'expected' behaviors/patterns are for these scenarios.)

[ittaiz]

Ok. I think since we're aiming for an actual fix in bazel itself we can move on.
Thanks for the explanation.