Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that the autodetected python runtimes are marked as compatible #20356

Closed
wants to merge 1 commit into from
Closed

Ensure that the autodetected python runtimes are marked as compatible #20356

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tools/python/toolchain.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

"""Definitions related to the Python toolchain."""

load("@local_config_platform//:constraints.bzl", "HOST_CONSTRAINTS")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa, what is this magic? What else is in this local_config_platform? This seems useful, though I'm not sure how yet...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • WORKSPACE and MODULE.bazel: Mark the repository (and depend on platforms 0.7.0).
  • BUILD.bazel: Declares the host platform, using the HOST_CONSTRAINTS from constraints.bzl
  • constraints.bzl: Creates the HOST_CONSTRAINTS, based on whatever Bazel autodetects as the host environment.

load(":utils.bzl", "expand_pyversion_template")

# TODO: move py_runtime_pair into rules_python (and the rest of @bazel_tools//python)
Expand Down Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? I would have expected a target_compatible_with constraints, similar to java_runtimes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to close this, then.

)

native.toolchain(
name = name + "_nonstrict",
toolchain = ":_autodetecting_py_runtime_pair_nonstrict",
toolchain_type = ":toolchain_type",
visibility = ["//visibility:public"],
exec_compatible_with = HOST_CONSTRAINTS,
)
Loading