Skip to content

fix: support py_cc_toolchain#1131

Draft
jbedard wants to merge 1 commit into
mainfrom
1095-inc-macos
Draft

fix: support py_cc_toolchain#1131
jbedard wants to merge 1 commit into
mainfrom
1095-inc-macos

Conversation

@jbedard

@jbedard jbedard commented Jun 19, 2026

Copy link
Copy Markdown
Member

Changes are visible to end-users: yes/no

  • Searched for relevant documentation and updated as needed: yes/no
  • Breaking change (forces users to change their own code or config): yes/no
  • Suggested release notes appear below: yes/no

Test plan

  • Covered by existing test cases
  • New test cases added
  • Manual testing; please provide instructions so we can reproduce:

@aspect-workflows

aspect-workflows Bot commented Jun 19, 2026

Copy link
Copy Markdown

✨ Aspect Workflows Tasks

📅 Fri Jun 19 20:28:35 UTC 2026

❌ 2 failed tasks

  • ❌ test (test-root-bazel-8) · ⏱ 2m 2s · 🐙 GitHub Actions · ☑️ Check
    💬 failed in test · Bazel test failed (1 test failed)
  • ❌ test (test-root-bazel-9) · ⏱ 2m 51s · 🐙 GitHub Actions · ☑️ Check
    💬 failed in test · Bazel test failed (1 test failed)

✅ 5 successful tasks

  • ✅ buildifier · ⏱ 22.2s · 🐙 GitHub Actions · ☑️ Check
    💬 Format complete (clean)
  • ✅ gazelle · ⏱ 27.4s · 🐙 GitHub Actions · ☑️ Check
    💬 Gazelle complete (clean)
  • ✅ test (test-e2e-bazel-8) · ⏱ 2m 20s · 🐙 GitHub Actions · ☑️ Check
    💬 Bazel test complete (150/150 passed)
  • ✅ test (test-e2e-bazel-9) · ⏱ 3m 7s · 🐙 GitHub Actions · ☑️ Check
    💬 Bazel test complete (145/145 passed · 128 cached)
  • ✅ test (test-examples-uv_pip_compile-bazel-8) · ⏱ 32.2s · 🐙 GitHub Actions · ☑️ Check
    💬 Bazel test complete (1/1 passed · 1 cached)

🔁 Reproduce

❌ test (test-root-bazel-8 · test-root-bazel-9)

aspect test -- //uv/private/uv_hub:snapshots_9_test
# vanilla bazel
bazel test -- //uv/private/uv_hub:snapshots_9_test

Install aspect: docs.aspect.build/cli/install


⏱ Last updated Fri Jun 19 20:32:29 UTC 2026 · 📊 GitHub API quota 23/15,000 (0% used, resets in 56m)
🚀 Powered by Aspect CLI (v2026.24.11)  |  Aspect Build · X · LinkedIn · YouTube

@github-actions

Copy link
Copy Markdown

py_binary startup benchmark

Version Mean (ms) Median (ms) ± stddev vs BCR vs main Build (s)
BCR 1.11.5 (baseline) 294.828 293.121 ±6.915 40.22
HEAD main 57.239 57.105 ±0.858 -80.6% 12.53
This PR 57.223 56.867 ±1.466 -80.6% -0.0% 11.25

Measured with hyperfine --warmup 5 --runs 50 on Linux
Gate: PR vs HEAD main (threshold: 10%). BCR is shown only as a historical baseline.
Build time: cold bazel build //:bench with isolated output base, no disk cache.

sys.path quality

Version sys.path entries distinct site-packages roots duplicate realpaths
BCR 1.11.5 (baseline) 6 1 0
HEAD main 7 2 0
This PR 7 2 0

sys.path quality measured by bench_syspath inside the assembled venv. Duplicate realpaths indicate symlink redundancy; many distinct site-packages roots suggest an inefficient venv layout.

@tamird

tamird commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

My clanker exercised this path through a rules_py 2 integration and
found that a compile-only test can silently resolve rules_python's
fallback C toolchain instead of the generated PBS one.

The replacement commit is
tamird@251eb97. It compares the
selected runtime and C-toolchain implementation directly, covers regular
and free-threaded selection, keeps the Windows full and stable import
libraries disjoint, and imports an extension whose PY_VERSION_HEX
must match sys.hexversion. Free-threaded toolchains omit
headers_abi3 because CPython rejects Py_LIMITED_API there.

That commit is currently stacked on several open prerequisite changes, so
I have not opened a competing PR. Are you okay with me superseding this
draft once those land, or would you prefer to fold the commit into this
branch?

@jbedard

jbedard commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

I'm not attached to this branch at all if you have an alternate. Your link is a 404 though...

@tamird

tamird commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

The reviewed replacement is now
tamird@4855688.
It verifies exact platform, runtime, and C-payload selection through public
provider data, exercises regular and free-threaded compile/import paths, and
does not advertise the stable ABI for free-threaded runtimes.

@ctcjab

ctcjab commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Downstream data point: this fixes a real nanobind_extension on Linux — not just the <Python.h> smoke test.

We hit #1095 in a monorepo that sources Python toolchains entirely from rules_py's python_interpreters (PBS) extension, with no direct rules_python dep, so nanobind's @rules_python//python/cc:current_py_cc_headers had no registered py_cc_toolchain.

Tested this branch (58fc7e46) via git_override on Linux x86_64 with a Zig hermetic cc toolchain, against a real nanobind_extension target:

  • Analysis resolves cleanly — the prior No matching toolchains found for @@rules_python+//python/cc:toolchain_type is gone.
  • The extension compiles + links against the PBS interpreter's headers and imports at runtime, passing under CPython 3.11, 3.12, and 3.13.

Because this drives the real current_py_cc_headers path (compile → link → import), it shouldn't be satisfiable by rules_python's fallback C toolchain — which I believe is the masking risk @tamird raised about the bare <Python.h> compile in the PR's e2e case. If it'd help, I'm happy to contribute this nanobind case as an e2e fixture (nanobind_extension → import smoke test, ~15 lines) so there's coverage that exercises the generated PBS toolchain end-to-end rather than a bare header compile.

Separately (not a claim about this PR): with #1095 previously blocking analysis, we'd never reached runtime to observe #1048 on this path — and at this commit we see no sign of the sys.base_prefix issue on any of 3.11/3.12/3.13.

This (or @tamird's alternative) reaching a mergeable state is the last blocker for our rules_py v2 adoption on native-extension packages, so glad to help validate — happy to re-run against 4855688 once its prerequisites land.

@ctcjab

ctcjab commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Correction + a sharper result after digging into toolchain resolution.

My earlier "shouldn't be satisfiable by rules_python's fallback C toolchain" was too strong — the masking is real, but version-dependent, and I can now show exactly where it does and doesn't bite.

Setup: python_interpreters as the rules_python drop-in; rules_python present only transitively (resolves to 1.7.0 here), its default pythons_hub registering py_cc toolchains for 3.11 only in this graph. Building a real nanobind_extension, per target Python version:

Target Py Without this PR With this PR
3.13 ❌ analysis fails: No matching toolchains found for @@rules_python+//python/cc:toolchain_type (rules_python's hub has no 3.13 py_cc) ✅ resolves @@aspect_rules_py++python_interpreters+…//:py_cc_toolchain
3.11 ⚠️ builds, but masked — resolves @@rules_python++python+python_3_11_…//:py_cc_toolchain, i.e. rules_python's own 3.11 headers, not the configured PBS interpreter ✅ resolves the PBS python_interpreters toolchain

So the NOTE in the e2e case is right that a real native build can be masked when rules_python is present — but only for the Python version(s) its transitive hub happens to register (3.11 here). For any other version (3.13) the gap is unmasked and the build genuinely fails without the registration. And even in the masked case the fix matters: without it, @rules_python//python/cc:current_py_cc_headers silently resolves rules_python's own interpreter headers rather than the PBS interpreter you configured — a latent header/runtime-interpreter mismatch (the concern @tamird raised, made concrete). With the PR, resolution correctly points at the python_interpreters toolchain across 3.11/3.12/3.13. All verified via --toolchain_resolution_debug.

Happy to turn this into the repro PR mentioned above: a real nanobind_extension e2e case that (a) fails on 3.13 without the registration and (b) asserts the selected py_cc toolchain is the python_interpreters one rather than rules_python's — so the masked-but-wrong 3.11 case is caught too. Let me know if you'd like it against e2e/cases/.

@ctcjab

ctcjab commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

One clarification for anyone finding this thread later: my testing above was against this branch (58fc7e46), which is now ~117 commits behind main. Current main already registers a py_cc_toolchain in python_interpreters (py/private/interpreter/repository.bzl) and ships a passing e2e/cases/pbs-cc-toolchain case that compiles a real extension against @rules_python//python/cc:current_py_cc_headers / current_py_cc_libs and asserts the exact PBS interpreter (plus abi3 / no-libpython-on-macOS) — so #1095 looks resolved on main via that route, not this branch.

Leaving my findings above for reference, but for anyone hitting the py_cc gap: point at main. I'm separately checking whether the nanobind-specific path needs any additional coverage on current main and will follow up if so.

@ctcjab

ctcjab commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the above: opened #1299 with the nanobind e2e case — a real nanobind_extension (a build_test plus a runtime import/call under a PBS 3.13 runtime) alongside pbs-cc-toolchain, covering the @rules_python//python/cc:current_py_cc_headers path the way most native extensions actually reach it. Confirms the py_cc registration on main works for the nanobind consumer and guards it against regression.

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.

3 participants