fix(packaging): build the .publish twine binary for the exec platform - #3909
fix(packaging): build the .publish twine binary for the exec platform#3909rdelfin wants to merge 1 commit into
Conversation
Sorry, something went wrong.
The .publish target generated by py_wheel wraps twine in a native_binary whose src attribute uses cfg = "target". When the build uses a cross-compilation platform (e.g. --platforms=linux_aarch64), this causes bazel to resolve twine for the target platform rather than the host, which fails because rules_python_publish_deps only provides wheels for the host platform. Twine is a developer tool that uploads wheels to PyPI; it only ever runs on the developer's workstation and should always be built for the exec platform regardless of the target. Replace the native_binary call with a minimal inline rule, _py_wheel_publish_binary, that is identical except its src attribute uses cfg = "exec".
There was a problem hiding this comment.
Code Review
This pull request replaces the usage of bazel_skylib's native_binary with a custom _py_wheel_publish_binary rule in python/packaging.bzl to ensure the twine binary is built for the execution platform. The review feedback suggests adding allow_files = True to the src attribute of the new rule to maintain compatibility with native_binary and prevent analysis-phase errors when passing file targets.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Sorry, something went wrong.
| "src": attr.label( | ||
| executable = True, | ||
| mandatory = True, | ||
| cfg = "exec", | ||
| ), |
There was a problem hiding this comment.
To ensure full compatibility with bazel_skylib's native_binary and allow passing pre-built binary files directly as src, the src attribute should explicitly set allow_files = True. Without this, passing a file target to src will result in an analysis-phase error.
| "src": attr.label( | |
| executable = True, | |
| mandatory = True, | |
| cfg = "exec", | |
| ), | |
| "src": attr.label( | |
| executable = True, | |
| allow_files = True, | |
| mandatory = True, | |
| cfg = "exec", | |
| ), |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR aims to make the .publish target generated by py_wheel reliably runnable during cross-compilation by ensuring the twine executable is built for the exec (host) platform, not the target platform.
Changes:
- Remove the @bazel_skylib native_binary dependency from python/packaging.bzl.
- Add an inline _py_wheel_publish_binary rule intended to mirror native_binary, but with src built using cfg = "exec".
- Switch the .publish target generation in py_wheel() to use _py_wheel_publish_binary.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| _py_wheel_publish_binary = rule( | ||
| implementation = _py_wheel_publish_binary_impl, | ||
| executable = True, | ||
| attrs = { | ||
| "src": attr.label( | ||
| executable = True, | ||
| mandatory = True, | ||
| cfg = "exec", | ||
| ), | ||
| "data": attr.label_list(allow_files = True), | ||
| "out": attr.string(), | ||
| }, |
aignas
left a comment
There was a problem hiding this comment.
+1 on the direction here and thank you for the fix.
We have lot's of analysis tests in the tests folder and it would be very useful to add a regression test for this. With AI help it should be very doable.
That and please add a news/3909.fixed.md file summarising the fixes here.
Sorry, something went wrong.
| **copy_propagating_kwargs(kwargs) | ||
| ) | ||
| elif twine: | ||
| py_binary( |
There was a problem hiding this comment.
Don't we need to update this as well?
Sorry, something went wrong.
rickeylev
left a comment
There was a problem hiding this comment.
This isn't doing quite what you think.
The exec platform is the platform bazel has chosen to run a build action. When remote execution is used, the exec platform can differ from the bazel host platform, e.g. a mac laptop using linux executors.
Additionally, the .public is intended to be used with bazel run, which means the output has to be for the target configuration (which is set to the host platform, since that is where bazel is going to run it).
With src using cfg=exec, and the result being symlinked as returned, what's potentially happening is src gets built for the exec platform, which might be linux. That linux result is then returned to the cfg=target .publish rule, which might be mac. Then bazel tries to run it, i.e. a mac tries to run a linux output. For pure python programs without platform-specific dependencies or native modules, this goes unnoticed. Similar occurs if there's no RBE setup with different platforms.
Hence, cfg=target is the correct config to use for twine.
The description mentions cross-compiling. If you want to, for example, run bazel run foo.publish and have "foo" built for a different platform, then what you want to do is apply a transition on the data that twine is processing.
Sorry, something went wrong.
|
Ah, i see, you want to do bazel run --platforms=linux_aarch64 //foo.publish (e.g. on a mac or x86) and have "foo" built for aarch64? Hmm. That is an ergonomic invocation and does feel like it expresses the intent. Maybe change py_wheel_publish_binary_impl to have a rule-level transition that sets --platforms=@platforms//host ? It's still going to inherit other command line flags (e.g. -c opt), but that would help nudge it into the same platform as the bazel host. Unfortunately bazel isn't particularly good at this situation. You might be better served by having two separate invocations and passing one to another, e.g. bazel build --platforms=linux_aarch64 foo_wheel bazel run //:twine -- bazel-bin/foo.wheel The above is just another way to do what I previously described (applying a transition to foo.wheel) |
Sorry, something went wrong.
|
Hi @rickeylev essentially yes. It seems to be how it was previously working, and the flow broke after I did an upgrade of rules_python, which is how I stumbled into this. Let me have a look at doing that. Maybe I should accept my fate and just have it as too commands. |
Sorry, something went wrong.
The .publish target generated by py_wheel wraps twine in a native_binary whose src attribute uses cfg = "target". When the build uses a cross-compilation platform (e.g. --platforms=linux_aarch64), this causes bazel to resolve twine for the target platform rather than the host, which fails because rules_python_publish_deps only provides wheels for the host platform.
Twine is a developer tool that uploads wheels to PyPI; it only ever runs on the developer's workstation and should always be built for the exec platform regardless of the target.
Replace the native_binary call with a minimal inline rule, _py_wheel_publish_binary, that is identical except its src attribute uses cfg = "exec".