Skip to content

feat: allow custom platform when overriding #2880

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.

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

rickeylev
Copy link
Collaborator

This basically allows using any python-build-standalone archive and using it
if custom flags are set. This is done through the single_version_platform_override()
API, because such archives are inherently version and platform specific.

Key changes:

  • The platform arg can be any value (mostly; it ends up in repo names)
  • Added target_compatible_with and target_settings args, which become the
    settings used on the generated toolchain() definition.

The platform settings are version specific, i.e. the key (python_version, platform)
is what maps to the TCW/TS values.

If an existing platform is used, it'll override the defaults that normally come
from the PLATFORMS global for the particular version. If a new platform is used,
it creates a new platform entry with those settings.

Along the way:

  • Added various docs about internal variables so they're easier to grok at a glance.

Work towards #2081

@rickeylev rickeylev requested a review from aignas as a code owner May 14, 2025 20:27
@rickeylev
Copy link
Collaborator Author

Notes to self:

The basic problem is it's trying to register the 3.13.3 linux-x86-install-stripped runtime as the host runtime. I don't want this to happen, but its pretty insistent. It does this because python.bzl computes a minor_mapping from seen versions and uses the highest patch version. Because the custom 3.13.3 is the highest 3.13 version, it starts treating 3.13 as 3.13.3, which only has the linux-x86-install-stripped runtime.

Thus, when it goes to create the python_3_13_host repo, it tries to use linux-x86-install-stripped. This fails because there's no platform for 3.13.3 that says its compatible with the host platform. That isn't strictly true in this particular case (my host is linux x86), but that's an easy case to make true (Ignas's host is mac, for example). Requiring that I register a runtime for every host my devs use doesn't make sense either -- the purpose of this feature is to add more esoteric versions and platforms; it's likely it'll be a newer python version and/or i don't care that every dev's host can use it.

So, I think the thing to do is change the host-repo creation to go looking a bit harder for a runtime that is compatible with the host os. Move the host_repo() creation out of python_register_toolchains (for bzlmod), and have python.bzl find something to call host_repo() with.

@aignas
Copy link
Collaborator

aignas commented May 15, 2025

Regarding the minor_mapping, you can explicitly tell the python thing to set a different minor_mapping value, I think.

@rickeylev
Copy link
Collaborator Author

manually set minor_mapping

I'll give that a try and see what happens, though I'd like things to work without having to do that.

What's really irking me is that I can't register a runtime unless it has host-compatible variants defined, even if it never gets used in a repository context.

@rickeylev
Copy link
Collaborator Author

I'm going to move this back to draft for now. I'm splitting out some separate PRs to refactor the internals that should help this PR and the python.configure() style API we talked about.

@rickeylev rickeylev marked this pull request as ready for review May 27, 2025 20:31
@rickeylev
Copy link
Collaborator Author

PTAL

tldr of notable changes since last review:

There's a second loop to find a host-compatible runtime if one wasn't created earlier (when python_register_toolchains is calle). Mainly this handles the case where adding a runtime (but only for a subset of platforms) with a higher version causes @python_3_13_host to try and use that higher version -- but a host compatible variant doesn't exist. To fix, keep track of all the host-compatible runtimes for the versions and fallback to one of them.

The runtimes within a version get registered in the order of the platform keys. This ends up mattering when custom flags are used for a runtime -- if the regular runtime comes first, it matches (same platform), and the one with custom flags doesn't get looked at. To fix, I made overrides get added as the first key in the platforms dict. (We should transform the map of platforms to a list or something, but something for another PR).

It's now possible to register a runtime and have no host-compatible version available. Thanks for the interpreters.bzl tip -- that was the spot that kept trying to reference things that didn't exist.

Some other misc changes:

  • Fixed a bug where overrides (of strip_prefix or urls) would try to modify an immutable dict -- default["tool_versions"] had an immutable copy from the global.
  • host_compatible_python_repo now takes the backing repo names as inputs (instead of making
    assumptions about the platform being the backing repo name)
  • single_version_platform_override accepts os_name/arch args. These populate target_compatible_with (if TCW is empty)

@rickeylev
Copy link
Collaborator Author

As an aside, this should lay the internal groundwork for registering runtimes in rule_python's MODULE.bazel directly (instead of TOOL_VERSIONS). That'll probably be easier if the TOOL_VERSIONS and PLATFORMS objects are further decoupled from the bzlmod extension (i.e. transform those into a simpler list[Thing] at the start instead of having to constantly key back into those objects and cross reference pieces between them).

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants