Support make variable expansion in environment variables when using python_register_multi_toolchains

[michaelboyd2]

🐞 bug report

Affected Rule

I think the problem is that _transition_py_impl does not handle make variable expansion of environment variables (it already does location expansion).

Is this a regression?

No, I don't think so.

Description

When you use py_test (and py_binary I guess) from a specific python version with Make variables for environment variables then they do not work e.g.

load("@python//3.8:defs.bzl", py_binary_3_8 = "py_binary", py_test_3_8 = "py_test")

py_test_3_8(
    ...
    env = {
        "JAVA_HOME": "$(JAVABASE)"
    },
    toolchains = ["@bazel_tools//tools/jdk:current_java_runtime"],

then $(JAVABASE) does not get expanded as it ought to.

🔬 Minimal Reproduction

See above. I could probably create a GitHub repo if it was needed.

🔥 Exception or Error

Depends on how the env variable is being used.

🌍 Your Environment

Operating System:

michaelboyd $ sw_vers
ProductName:    macOS
ProductVersion: 12.6.9
BuildVersion:   21G726

Output of bazel version:

michaelboyd $ bazel version
Bazelisk version: development
Build label: 6.3.2
Build target: bazel-out/darwin_arm64-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Tue Aug 8 15:51:44 2023 (1691509904)
Build timestamp: 1691509904
Build timestamp as int: 1691509904

Rules_python version:

0.23.1

I have not tested a more up to date version, but I think it would happen on main too.

Anything else relevant?

No.

[michaelboyd2]

At the moment, I am using this patch to work around this issue:

--- python/config_settings/transition.bzl
+++ python/config_settings/transition.bzl
@@ -58,6 +58,10 @@ def _transition_py_impl(ctx):
         )
     env = {}
     for k, v in ctx.attr.env.items():
+        if v.startswith("$(") and v.endswith(")"):
+            v = v[2:-1]
+            v = ctx.var[v]
+
         env[k] = ctx.expand_location(v)

     providers = [

I would be happy to contribute towards a fix, but I am not that confident that I can fix in the best possible way.

[rickeylev]

A full fix will have to wait until Bazel 7. In Bazel 7, a special API is made available to rules_python to allow its regular Starlark code to expand these mixed make+location expressions (see how python/private/common/py_executable.bzl expands the env attribute)

For prior versions of Bazel, we'd have to either manually parse the string and re-implemented the equivalent logic, or backport the special api to the earlier Bazel release (probably possible, but not sure its worth the effort).

[github-actions]

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

[michaelboyd2]

This still needs doing. In fact, Bazel 7 is out now so perhaps a proper fix can be made. I am thinking of having a go at this using expand_location_and_make_variables as suggested above. @rickeylev let me me if you have any other thoughts or tips that would help me go about this in the proper way. Thanks!

[aignas]

Since this is a request towards the non-bzlmod APIs, please open a new ticket if needed for the bzlmod APIs we have. If you would like to contribute PRs towards this toolchain registration, feel free, we will review them.