Repository navigation
Fix dds3 wheel CPython/platform tags; smoke-test pip install in CI - #440
Merged
Merged
Conversation
py_wheel defaulted dds3_wheel to python_tag="py3", abi="none", platform="any", mislabeling the compiled _dds3 extension as a universal pure-Python wheel pip would install (and then fail to import) anywhere. Tag it cp314/cp314 to match MODULE.bazel's single pinned toolchain, and platform-select an honest per-OS tag (linux_x86_64, macosx_11_0_arm64, win_amd64) rather than overclaiming manylinux/arch portability this build doesn't actually provide. Add ci_wheel_tags_test to guard the tags, and ci_wheel_smoke_test plus a "Build and smoke-test Python wheel" CI step on all three platforms to actually pip install and import the packaged .whl, since neither `bazelisk build //...` nor `bazelisk test //...` exercises that path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The config-setting matrix spec didn't mention the build_macos_arm64 config_setting added for dds3_wheel's platform tag, even though the spec claims to record the full matrix. Per review feedback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oke test in the spec. Get-ChildItem | Select-Object -First 1 silently yields $null if no .whl matches, and $null.FullName then silently evaluates to $null rather than failing with a clear message. Added a failing ci_wheel_smoke_test assertion first, then the Write-Error/exit guard to make it pass. Also name ci_wheel_smoke_test, not just ci_wheel_tags_test, as guarding the wheel-tagging spec bullet, since it's the test that actually drives the three CI pip-install steps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Linux ARM64 wheels are mislabeled as x86-64, while macOS ARM64 wheels advertise an unenforced deployment target, risking installation rejection or import failures.
2 open issues
What changed in this PR
This PR replaces the native dds3 wheel’s universal tags with CPython 3.14 and platform-specific tags, and adds CI checks that install and import the packaged wheel.
Changes:
- Adds explicit wheel tags and an ARM64-specific macOS build selector.
- Adds isolated wheel installation smoke tests on Linux, macOS, and Windows, plus regression guards.
- Updates documentation to describe the wheel’s CPython 3.14 requirement.
| File | Description |
|---|---|
specs/python-binding.md |
Documents wheel compatibility and CI guards. |
specs/build-system.md |
Documents the macOS ARM64 selector. |
python/tests/ci_windows_cppopts_test.py |
Scopes build assertions to the existing native build step. |
python/tests/ci_wheel_tags_test.py |
Adds guards for CPython and platform tags. |
python/tests/ci_wheel_smoke_test.py |
Guards wheel installation and import steps across CI workflows. |
python/BUILD.bazel |
Sets wheel tags and registers regression tests. |
docs/python_interface.md |
Clarifies build and wheel interpreter requirements. |
BUILD.bazel |
Adds the macOS ARM64 configuration setting. |
.github/workflows/ci_windows.yml |
Adds Windows wheel installation and import checks. |
.github/workflows/ci_macos.yml |
Adds macOS wheel installation and import checks. |
.github/workflows/ci_linux.yml |
Adds Linux wheel installation and import checks. |
Review effort: Balanced
//:build_linux matched any Linux CPU, so an aarch64 Linux build would still get the "linux_x86_64" platform tag -- pip would reject the wheel on its real architecture and accept it on an incompatible one. Add a CPU-qualified build_linux_x86_64 config_setting (mirroring the existing build_macos_arm64 fix for the analogous macOS case) and key the wheel's platform select on it, failing closed on other Linux architectures like build_windows already does for non-x86_64. Addresses Copilot review comment on PR #440 (python/BUILD.bazel:339). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The wheel targets break normal builds on Linux ARM64 and Intel macOS, while the macOS ARM64 wheel claims macOS 11 compatibility without pinning or verifying that deployment target.
1 open issue
2 resolved since last review
Review effort: Balanced
dds3_wheel tags macOS builds "macosx_11_0_arm64", but nothing previously constrained the compiled extension's actual minimum OS version -- the real floor baked into the .so was whatever the runner's default Xcode SDK picked. Set -mmacosx-version-min=11.0 in both DDS_CPPOPTS and DDS_LINKOPTS for build_macos/debug_build_macos so the tag is backed by an enforced floor, not an assumption. Verified via `otool -l` that the built _dds3.so now carries LC_BUILD_VERSION minos 11.0, and that a full `bazelisk build //...` / `bazelisk test //...` pass is still clean -- prebuilt externals like googletest keep their own default target and emit a harmless `ld: warning` on test binaries that link both, which doesn't affect results. Added ci_macos_deployment_target_test (TDD: written first, confirmed failing against the unmodified CPPVARIABLES.bzl) to guard both that the flag is set and that its floor matches the wheel's advertised tag. Addresses Copilot review comment on PR #440 (python/BUILD.bazel:350). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The wheel platform select() fails closed (no arm) on Linux ARM64 and Intel macOS by design, but that meant a plain `bazelisk build //...` or `bazelisk test //...` on those hosts hit an analysis-time error on this unrequested wheel target, even for a developer who only wants a source build. docs/python_interface.md documents //python:_dds3 itself as buildable on other Linux architectures via toolchain fallback, so this was a regression for anyone on those hosts. Add tags = ["manual"] to both targets so they're excluded from //...'s target pattern expansion. CI is unaffected: all three workflows already build dds3_wheel_dist explicitly by name, which manual does not touch. Updated the now-stale "bazelisk build //... already produces dds3_wheel_dist as a side effect" comments/docstring in the three CI workflows and ci_wheel_smoke_test.py accordingly. Added ci_wheel_tags_test.TestDds3WheelManualTag (TDD: written first, confirmed failing against the unmodified BUILD.bazel) to guard both targets carry the tag. Addresses Copilot review comment on PR #440 (python/BUILD.bazel:333). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR #440's "sanitizers" CI check failed: --config=asan on macOS switches to local_config_apple_cc_toolchains (see specs/build-system.md -- LLVM's bundled ASAN runtime hangs at startup), whose generated compile command already carries its own `-target arm64-apple-macosX.Y`. The -mmacosx-version-min=11.0 added in c8e5471 conflicts with that under -Werror: clang++: error: overriding '-mmacosx-version-min=11.0' option with '-target arm64-apple-macosx27.0' [-Werror,-Woverriding-option] Add debug_build_macos_asan, a config_setting strictly more specific than debug_build_macos (matched via a --define build:asan_macos now sets), so CPPVARIABLES.bzl's select() picks an arm without the conflicting flag only under asan -- release and plain debug macOS builds are unaffected. Verified via `bazelisk build --config=asan` on the previously-failing targets and `bazelisk test --config=asan //library/tests/system/...` (22/22 pass, was failing to build); also reran a full `bazelisk build //...` and `bazelisk test //python/...` to confirm no regression. TDD: python/tests/ci_macos_deployment_target_test.py's new TestAsanMacosAvoidsConflictingDeploymentTargetFlag was written first and confirmed failing (missing config_setting/define/arms) before the fix. The other PR #440 CI failure (almalinux:10, "No space left on device") is a GitHub-hosted-runner disk exhaustion, not caused by this branch; not addressed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The ASAN compile guard can falsely pass by matching a later variable’s assignment, so the search must be scoped to the named assignment and covered by a regression test.
1 open issue
1 resolved since last review
Review effort: Balanced
It searched text[var_match.start():] -- from the named variable to the end of the file -- rather than stopping at the next top-level `name = ` assignment. A label missing from the named variable's own select() but present in some later variable's select() was silently returned instead of raising, so e.g. removing debug_build_macos_asan from DDS_CPPOPTS while it still existed (empty) in DDS_LINKOPTS would let test_cppopts_asan_arm_has_no_deployment_target_flag pass even though compilation would then fall through to debug_build_macos's conflicting -mmacosx-version-min flag. Bound the search to [var_match.start(), next top-level assignment or EOF). TDD: added test_does_not_leak_into_a_later_variables_select first, confirmed it failed (returned '' instead of raising) against the unbounded search, then fixed. Addresses Copilot review comment on PR #440 (python/tests/ci_macos_deployment_target_test.py:45). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzcgumn
added
the
Clean Copilot review
Collaborator
Author
|
@tameware, this is to make sure that it is easy to install the python extension on all platforms. Copilot is happy with this, are you for this to be merged? |
tameware
approved these changes
Nov 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dds3_wheelwas taggedpy3-none-anyby py_wheel's defaults despite packaging a compiled pybind11 extension; it now carries honestcp314/cp314and per-OSplatformtags (linux_x86_64,macosx_11_0_arm64,win_amd64), matchingMODULE.bazel's single pinned CPython 3.14 toolchain.build_macos_arm64rootconfig_setting(CPU-qualified sibling ofbuild_macos) to back the macOS arm of that tagselect(), and documented it inspecs/build-system.md.pip install --no-indexs the packaged.whlinto a throwaway venv and importsdds3, since neitherbazelisk build //...norbazelisk test //...exercises a realpip install.ci_wheel_tags_test(guards the tags) andci_wheel_smoke_test(guards the three CI steps); updateddocs/python_interface.mdandspecs/python-binding.mdto state the wheel is CPython-3.14-exact instead of the now-false "3.10+" claim.Get-ChildItemresult in the Windows step so a missing.whlfails loudly instead of silently passing$null.FullNametopip install.Test plan
python3 -m unittest python.tests.ci_wheel_tags_test python.tests.ci_wheel_smoke_test python.tests.ci_windows_cppopts_test— 60 tests pass🤖 Generated with Claude Code