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
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.
| "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", | |
| ), |
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_skylibnative_binarydependency frompython/packaging.bzl. - Add an inline
_py_wheel_publish_binaryrule intended to mirrornative_binary, but withsrcbuilt usingcfg = "exec". - Switch the
.publishtarget generation inpy_wheel()to use_py_wheel_publish_binary.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _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.
| **copy_propagating_kwargs(kwargs) | ||
| ) | ||
| elif twine: | ||
| py_binary( |
There was a problem hiding this comment.
Don't we need to update this as well?
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.
|
Ah, i see, you want to do 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 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. The above is just another way to do what I previously described (applying a transition to foo.wheel) |
|
Hi @rickeylev essentially yes. It seems to be how it was previously working, and the flow broke after I did an upgrade of |
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".