fix(toolchains): correctly register musl/freethreaded toolchains for workspace #3314
Conversation
Summary of ChangesHello @rickeylev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue where musl and freethreaded Python runtimes were not being properly activated in Bazel WORKSPACE builds. The core problem stemmed from missing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where musl and freethreaded toolchains were not being properly activated in WORKSPACE builds by ensuring target_settings are passed during toolchain registration. This change aligns the WORKSPACE behavior with the bzlmod implementation.
A significant portion of this PR involves refactoring the test infrastructure to centralize platform definitions, which greatly improves maintainability. Additionally, a comprehensive new test suite has been added to verify correct toolchain resolution across various configurations, which is an excellent addition to ensure the fix is robust.
I've found one critical issue in the new test implementation that would likely cause it to fail and have provided a suggestion for a fix. Overall, this is a high-quality contribution.
rickeylev
force-pushed
the
fix.musl.ft.toolchain.registration.workspace
branch
from
4af90ca to
bf68216
Compare
[Design doc](https://docs.google.com/document/d/1UFessu8_BwmfMJXDvhyhaASrNMo7_3-gO1lTVASbJbs) An implementation of pip based on consuming PEP-751 [1] like lockfiles. Specifically uv lockfiles, which contain internal dependency graph information that the PEP-751 specification labels optional. Follows in the footsteps of rules_js's pnpm support by consuming a lockfile which contains enough information to produce materialize dependencies without performing _any_ repository time operations which could be platform dependent. ## Features - Supports cross-platform builds of wheels - Supports hermetic source builds of wheels - Automatically handles dependency cycles - Creates unified pip hubs which span virtualenv/dependency solution boundaries - Pip library targets from deactivated venvs are _incompatible_ - Platform constraints on pip libraries do not prevent the creation of a library target - Allows for `--editable`-like workflows - Supports `console_scripts` entrypoints* ## Example ``` # MODULE.bazel content uv = use_extensioin("@aspect_rules_py//uv:extesion.bzl", "uv") uv.declare_hub(hub_name = "pip") uv.declare_venv(hub_name = "pip", venv_name = "a") uv.lockfile(hub_name = "pip", venv_name = "a", src = "third_party/py/venvs/uv-a.lock") uv.declare_venv(hub_name = "pip", venv_name = "b") uv.lockfile(hub_name = "pip", venv_name = "b", src = "third_party/py/venvs/uv-b.toml") use_repo(pip, "pip") ``` ``` # BUILD.bazel content py_venv_binary( name = "foo", srcs = [ "foo.py", ], main = "foo.py", deps = [ "@pip//cowsay", # Pull cowsay from the configured venv ], venv = "a", # Configure the default venv to be "a"; may be overriden at the CLI ) ``` The active venv state can be overriden at the cli by specifying `--'@pip//venv=b'` here for instance, or by using transitions to(re) set that same flag. ## Appendix [1] https://peps.python.org/pep-0751/ [2] https://peps.python.org/pep-0751/#locking-build-requirements-for-sdists ### Changes are visible to end-users: yes - Searched for relevant documentation and updated as needed: no - Breaking change (forces users to change their own code or config): no - Suggested release notes appear below: yes `aspect_rules_py//uv` is now available as an alternative to `pip.parse` ### To do list - ~[x] Audit the comments/FIXMEs for accuracy~ fast-follow - ~[x] Go over zbarsky's nits~ mostly rejected for pythonstyle - ~[x] Create a quick and dirty `uv_lock` rule~ fast-follow - [x] Document the extension - [x] Document that we consider the extension is unstable - [x] Replace `pip.parse` internally entirely - [x] Add support for annotating _replacement_ of pip deps with internal builds (`--editable` / vendoring) - [x] Implement an "all deps for all venvs" data source comparable to the `rules_python` equivalent - [x] Go back over the interpreter compatibility machinery and align it with `rules_python`'s config settings for now - [x] Go back over the interpreter feature flags and align it with `rules_python`'s config settings for now bazel-contrib/rules_python#3314 - ~Investigate implementing a `dist_info` target comparable to that which `rules_python` generates~ There isn't a great way to do this because dist-info is in our world part of the installed package, so any such target is just ignoring the PyInfo details on the fileset. - [x] Provide a pattern for implementing/activating venv entrypoints - [x] Implement conditional activation of deps - [x] Implement an "all active deps" target comparable to the `rules_python` all list - [x] Look into platform conditional deps & how they get represented - [x] Flatten the git log - [x] Ensure `python_version` transition consistency with `rules_python` - [x] Consider a feature flag to turn on `rules_python`'s [package name normalization](https://github.com/bazel-contrib/rules_python/blob/main/python/private/normalize_name.bzl) so that migration is easier. - [x] Implement extra activation - [x] Match the `rules_python` `@hub//package[:package]` syntax? - [X] Get a `toml.bzl` working - [X] Toolchainize the `uv` dependency ### Test plan - [X] Manually test flipping the venv command line flag - [X] Manually test flipping the venv transition attr - [x] Create `py_venv_test`s covering that different versions of the same package can be concurrently configured via different venvs - [x] Create a `py_venv_test` covering that Airflow or another package with dependency cycles can be provisioned - [x] Create a `py_venv_binary` embedded in and transitioned for a Linux OCI container across arch boundaries - [x] Create a `py_venv_test` covering overriding a pip dep with a 1stparty target --------- Co-authored-by: aspect-marvin[bot] <[email protected]>
2 participants
The musl/freethreaded runtimes weren't being activated when the flags were set. This was
because the toolchains weren't having
target_settingsset, which means extra settings,such as musl/freethreaded-ness were ignored when matching. The net result is the regular
toolchain, because it's registered earlier, would always match earlier.
To fix, set the target_settings in the toolchain() call. This matches the bzlmod behavior.
Also update the toolchain resolution tests to verify resolution.
Fixes #3262