Add make variables for runfiles location of $(JAVABASE) to support --no_legacy_external_runfiles. by thirtyseven · Pull Request #272 · bazelbuild/rules_java

thirtyseven · 2025-02-06T16:51:40Z

Add make variables for runfiles locations of $(JAVA) and $(JAVABASE). These are needed to access these paths when passing them to test rules via env when using --no_legacy_external_runfiles. The original make variables still work in exec contexts such as genrules.

Add make variables for runfiles locations of $(JAVA) and $(JAVABASE). These are needed to access these paths when passing them to test rules via `env`.

fmeum · 2025-02-06T17:33:16Z

@meteorcloudy We might want to cherry-pick this into 8.1.0 as it makes it easier to adopt the --nolegacy_external_runfiles flip.

We could then also document it on the Make variables page.

meteorcloudy · 2025-02-06T17:39:38Z

@fmeum Do you mean bump the rules_java version in MODULE.tools to include this change? End users can also just depend on a newer rules_java to get this?

fmeum · 2025-02-06T17:53:53Z

@fmeum Do you mean bump the rules_java version in MODULE.tools to include this change? End users can also just depend on a newer rules_java to get this?

That is correct, but if we don't bump the minimum requirement, we can't document these Make variables on the versioned page for 8.1.0.

meteorcloudy · 2025-02-07T10:21:13Z

OK, let's try to get this reviewed first, then let's see if we can make it for Bazel 8.1

@hvadehra Can you take a look?

hvadehra

Please also add a test for this. Easiest might be to use this somehow in https://github.com/bazelbuild/rules_java/tree/master/test/repo

hvadehra · 2025-02-07T13:34:23Z

toolchains/java_toolchain_alias.bzl

        platform_common.TemplateVariableInfo({
            "JAVA": str(toolchain.java_executable_exec_path),
            "JAVABASE": str(toolchain.java_home),
+            "JAVA_RUNFILES": str(toolchain.java_executable_runfiles_path),


I think we should instead be adding these to

rules_java/java/common/rules/java_runtime.bzl

Line 152 in af504cf

platform_common.TemplateVariableInfo({

and then just forward that provider like we do below in this file:

rules_java/toolchains/java_toolchain_alias.bzl

Line 46 in af504cf

template_variable_info = runtime[platform_common.TemplateVariableInfo]

In order to make this change, I think I would have to make java_runtime_alias and java_host_runtime_alias identical by adding a dependency on //toolchains:current_java_runtime to java_runtime_alias. I can do this, but wanted to make sure it's your intent first.

My bad, please ignore the bit about just forwarding the provider. But I do think we need to make the same change also in rules_java/java/common/rules/java_runtime.bzl

I have made the requested changes, however the test I added is failing due to toolchain.java_executable_runfiles_path appearing not to be a valid path in the context of the test. It's returning a path like:

/private/var/tmp/_bazel_thirtyseven/43339e4e51eaab2f90ecf6c85b4023b4/sandbox/darwin-sandbox/49/execroot/_main/../rules_java++toolchains+local_jdk/bin/java

which does not exist when running the test.

I'm not sure if this is due to something missing from my test or if I'm misusing this field, any help would be appreciated.

I have removed JAVA_RUNFILES from this PR for now. It seemed unlikely that it was going to work as I intended (making a Java executable easily available to test rules).

Realized I could do something like:

"JAVA_RUNFILES": toolchain.java_home_runfiles_path + "/bin/java",

it's a little ugly but works. Thoughts?

Does that work? I would think the issue is that JAVA_RUNFILES is a special env var¹ so if we'll want to use a different name.

Footnotes

https://github.com/bazelbuild/bazel/blob/d796714abba0a5531edf795c53a3985300577303/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java#L82 ↩

Not sure what a better name would be, I'm ok with leaving it out letting the user locate bin/java.

hvadehra · 2025-02-10T10:34:13Z

Please fix the buidifier failure, the sh rules have been moved out of Bazel, so we'll need to load them from @rules_sh

Load rules_shell Load rules_shell in WORKSPACE mode Buildify WORKSPACE Fix WORKSPACE

fmeum · 2025-02-11T08:51:29Z

java/common/rules/java_runtime.bzl

        platform_common.TemplateVariableInfo({
            "JAVA": java_binary_exec_path,
            "JAVABASE": java_home,
+            "JAVABASE_RUNFILES": java_home_runfiles_path,


Not adding an equivalent for the java binary makes this difficult to use cross-platform (think Windows).

I don't think the deprecated env var JAVA_RUNFILES would necessarily conflict with a make var. It's something we could likely just get rid of at this point anyway.

But to avoid confusion, how about calling this JAVA_ROOTPATH or JAVABASE_ROOTPATH to match $(rootpath ...)?

Sure, I've tried to address this.

fmeum · 2025-02-11T23:18:59Z

java/common/rules/java_runtime.bzl

        platform_common.TemplateVariableInfo({
            "JAVA": java_binary_exec_path,
            "JAVABASE": java_home,
+            "JAVA_ROOTPATH": paths.join(java_home_runfiles_path, "bin/java"),


This won't work on Windows. Could you use java_binary_runfiles_path instead? If that somehow isn't the right path, you could also reuse the basename of the execpath.

Done, java_binary_runfiles_path does actually work, the earlier issue I had must have been due to the JAVA_RUNFILES name conflict.

Add make variables for runfiles locations of $(JAVA) and $(JAVABASE)

365b924

Add make variables for runfiles locations of $(JAVA) and $(JAVABASE). These are needed to access these paths when passing them to test rules via `env`.

thirtyseven requested review from a team and comius as code owners February 6, 2025 16:51

thirtyseven mentioned this pull request Feb 6, 2025

rules_java seem to not work with --nolegacy_external_runfiles flag #203

Open

hvadehra requested changes Feb 7, 2025

View reviewed changes

thirtyseven added 2 commits February 7, 2025 09:23

Make requested changes, add tests

95f9d06

Remove JAVA_RUNFILES

e8bdf76

thirtyseven changed the title ~~Add make variables for runfiles locations of $(JAVA) and $(JAVABASE) to support --no_legacy_external_runfiles.~~ Add make variables for runfiles location of $(JAVABASE) to support --no_legacy_external_runfiles. Feb 8, 2025

Buildifier lint fix

fe185dc

Load rules_shell Load rules_shell in WORKSPACE mode Buildify WORKSPACE Fix WORKSPACE

thirtyseven force-pushed the patch-2 branch from cf517e6 to fe185dc Compare February 10, 2025 15:54

hvadehra previously approved these changes Feb 11, 2025

View reviewed changes

fmeum suggested changes Feb 11, 2025

View reviewed changes

thirtyseven dismissed hvadehra’s stale review via b66bda2 February 11, 2025 21:13

Rename to JAVABASE_ROOTPATH, add JAVA_ROOTPATH

0745b3a

thirtyseven force-pushed the patch-2 branch from b66bda2 to 0745b3a Compare February 11, 2025 21:14

fmeum reviewed Feb 11, 2025

View reviewed changes

Use toolchain.java_executable_runfiles_path

438b659

thirtyseven force-pushed the patch-2 branch from bfd8938 to 438b659 Compare February 12, 2025 00:23

fmeum approved these changes Feb 12, 2025

View reviewed changes

hvadehra approved these changes Feb 12, 2025

View reviewed changes

copybara-service bot closed this in 31edc44 Feb 13, 2025

FrankPortman mentioned this pull request Apr 1, 2025

make variables for runfiles location of $(PYTHON3) to support --no_legacy_external_runfiles bazel-contrib/rules_python#2728

Closed

Conversation

thirtyseven commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meteorcloudy commented Feb 6, 2025

Uh oh!

fmeum commented Feb 6, 2025

Uh oh!

meteorcloudy commented Feb 7, 2025

Uh oh!

hvadehra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thirtyseven Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thirtyseven Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvadehra commented Feb 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thirtyseven commented Feb 6, 2025 •

edited

Loading

fmeum commented Feb 6, 2025 •

edited

Loading

thirtyseven Feb 7, 2025 •

edited

Loading

thirtyseven Feb 8, 2025 •

edited

Loading