Fix rules_scala JDK lookup when using --nolegacy_external_runfiles.

[srdo-humio]

java_executable_exec_path does not look up the right path when resolving
the JDK from the runfiles directory and legacy_external_runfiles is disabled.
java_executable_runfiles_path should be used instead.

Examples of these paths when running "bazel build test:ScalaBinaryInGenrule":

java_executable_exec_path: external/remotejdk11_macos/bin/java
java_executable_runfiles_path: ../remotejdk11_macos/bin/java

The working directory appears to be
ScalaBinary.runfiles/io_bazel_rules_scala.

When legacy_external_runfiles is enabled, the JDK is present in
ScalaBinary.runfiles/remotejdk11_macos but is also linked
in ScalaBinary.runfiles/io_bazel_rules_scala/external/remotejdk11_macos,
so looking up using either exec_path or runfiles_path is correct.

When legacy_external_runfiles is disabled, the JDK only goes in
ScalaBinary.runfiles/remotejdk11_macos, so only runfiles_path will work.

Fixes #1396

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[srdo-humio]

As a followup to this, I think it would be good to ensure that --nolegacy_external_runfiles is set when running tests.

[liucijus]

@wiwa, @wisechengyi could you take a look at this one?

[liucijus]

@srdo-humio, thanks for the contribution, would you be able to contribute a test for this change? Or do you think we should set --nolegacy_external_runfiles globally?

[srdo-humio]

@liucijus It is my impression that --nolegacy_external_runfiles is supposed to become the default at some point, setting it globally might make sense?

If you would rather not set it globally, I think running (some of) the tests in both modes should be good enough to check that this doesn't regress. Let me know if you'd prefer handling it this way, and I'll add the extra check to test_rules_scala.sh.

Also apologies for the CLA check being red, I'm working on getting it signed internally.

[pcj]

Also hit this while (still attempting) to upgrade rules_scala to latest. In this repo, the rules_scala version was very old (circa 2020), but somehow was working with bazel 5.1.1. After attempting to upgrade rules_scala to latest, hit this issue.

Confirming that this change fixed the issue:

-    java_path = str(java_runtime.java_executable_exec_path)
+    java_path = str(java_runtime.java_executable_runfiles_path)

In our case, the presence/absence of --nolegacy_external_runfiles did not seem to matter.

Currently using this as:

    http_archive(
        name = "io_bazel_rules_scala",
        sha256 = "6e9191363357d30b144e7306fec74deea2c7f1de63f3ed32028838116c239e8a",
        strip_prefix = "rules_scala-4ba3780fcba8d26980daff4639abc6f18517308b",
        urls = ["https://github.com/bazelbuild/rules_scala/archive/4ba3780fcba8d26980daff4639abc6f18517308b.tar.gz"],
        patch_args = ["-p1"],
        patches = ["//:patches/rules_scala_PR_1397.patch"],
    )

[liucijus]

@srdo-humio will you sign CLA?

[simuons]

Hi @srdo-humio, thanks for contribution and sorry for delay.

To my understanding your change is correct. Since this javabin path is embedded into wrapper script it should point to runfiles.

Did a quick check with java_binary and it points to runfiles without external part.

If you could just add a single test for bazel build test:ScalaBinaryInGenrule --nolegacy_external_runfiles to catch such regression that would be awesome.

[srdo-humio]

I've added the test. I'm still working on getting the corporate CLA signed, best guess is we'll get it done in the next few days.

[srdo-humio]

Sorry for the delay, the CLA check is green now, so this might be ready to merge :)