Ensure that the autodetected python runtimes are marked as compatible #20356
Conversation
with the host platform. This should prevent them from being used inappropriately in remote execution. Part of changes for bazelbuild#20354.
github-actions
bot
added
the
awaiting-review
|
This doesn't seem to fix #20354 , should it? |
|
It addresses the smaller part brought up there, which was the lack of constraints. The prioritization of toolchains is still a problem. |
| @@ -242,11 +243,13 @@ def define_autodetecting_toolchain( | |||
| toolchain = ":_autodetecting_py_runtime_pair", | |||
| toolchain_type = ":toolchain_type", | |||
| visibility = ["//visibility:public"], | |||
| exec_compatible_with = HOST_CONSTRAINTS, | |||
There was a problem hiding this comment.
Is this correct? I would have expected a target_compatible_with constraints, similar to java_runtimes.
There was a problem hiding this comment.
The auto detecting toolchain is a bit funny because it basically just takes python from PATH. If you unroll the generated code, it basically does this:
py_runtime(
interpreter = "foo.sh"
)
# foo.sh
py=$(which python)
exec $py $@
So "compatible with every platform" (for both target and exec) is technically true. Usage of this toolchain basically means you're just assuming whatever system it runs on provides python. I'm not saying doing this is a good idea, its just what is historically there.
There was a problem hiding this comment.
I see, then the "no constraint" toolchain seems actually correct. The "autodetecting_*" bit threw me off as I thought the detection would apply at repository rule time.
There was a problem hiding this comment.
Happy to close this, then.
There was a problem hiding this comment.
I'm not entirely convinced any constraints should be set.
The auto detecting toolchain is a bit funny because it basically just takes python from PATH. If you unroll the generated code, it basically does this:
py_runtime(
interpreter = "foo.sh"
)
# foo.sh
py=$(which python)
exec $py $@
Stated another way: the python interpreter to use is selected at runtime, not build time. The local system isn't e.g. copying its binary over to run on a remote system.
So "compatible with every platform" (for both target and exec) is technically true. Usage of this toolchain basically means you're just assuming whatever system it runs on provides python. I'm not saying doing this is a good idea, its just what is historically there.
Also, fwiw, I've been trying to get rid of this toolchain and the auto-registration of it, but that got hung up on various test failures.
| @@ -14,6 +14,7 @@ | |||
|
|
|||
| """Definitions related to the Python toolchain.""" | |||
|
|
|||
| load("@local_config_platform//:constraints.bzl", "HOST_CONSTRAINTS") | |||
There was a problem hiding this comment.
Whoa, what is this magic? What else is in this local_config_platform? This seems useful, though I'm not sure how yet...
There was a problem hiding this comment.
Just this, but it is very useful for e.g. marking rules that should always resolve toolchains for the host via exec_compatible_with. We use this trick in rules_go to make bazel run @rules_go//go always run the correct go tool even with --platforms.
There was a problem hiding this comment.
The magic is in LocalConfigPlatformFunction, but it's really easy (and could be fairly trivially migrated to a Starlark repo rule in the builtins, I bet).
The repo creates the following files:
WORKSPACEandMODULE.bazel: Mark the repository (and depend on platforms 0.7.0).BUILD.bazel: Declares thehostplatform, using theHOST_CONSTRAINTSfromconstraints.bzlconstraints.bzl: Creates theHOST_CONSTRAINTS, based on whatever Bazel autodetects as the host environment.
| @@ -242,11 +243,13 @@ def define_autodetecting_toolchain( | |||
| toolchain = ":_autodetecting_py_runtime_pair", | |||
| toolchain_type = ":toolchain_type", | |||
| visibility = ["//visibility:public"], | |||
| exec_compatible_with = HOST_CONSTRAINTS, | |||
There was a problem hiding this comment.
The auto detecting toolchain is a bit funny because it basically just takes python from PATH. If you unroll the generated code, it basically does this:
py_runtime(
interpreter = "foo.sh"
)
# foo.sh
py=$(which python)
exec $py $@
So "compatible with every platform" (for both target and exec) is technically true. Usage of this toolchain basically means you're just assuming whatever system it runs on provides python. I'm not saying doing this is a good idea, its just what is historically there.
|
Closing this as unneeded. |
github-actions
bot
removed
the
awaiting-review
with the host platform.
This should allow other python runtimes to be registered and used with bzlmod.
Part of changes for #20354.