fix(uv): select configured native build tools#1328
Conversation
PEP 517 backends change into the unpacked source tree, so relative tool and dependency paths from the action execroot stop resolving. Action-only C++ toolchains can also expose synthetic CC, AR, LD, and STRIP Make-variable paths that are not declared inputs. Select the configured compiler, archiver, linker, and strip tools from the C++ action configuration, preserve explicit overrides, and expose $(EXECROOT) for dependency paths in arbitrary environment flags. Resolve markers and tool paths before the backend changes directory. Add native compile, archive, and link regressions with an in-repo C library, anchored and bare tool overrides, and marker-collision coverage. Remove obsolete generated defaults and refresh snapshots. Co-authored-by: Connor McEntee <connor.mcentee@cloudsoda.io>
Use the configured C and C++ compile-action tools for native sdist builds, including toolchains with nonstandard driver names. Keep the minimal ToolchainInfo fallback and explicit environment overrides, and remove the obsolete filename heuristics.
Use the legacy ar_executable when a C++ toolchain has no enabled static link action, and omit unavailable action tools instead of publishing empty environment values. Add an end-to-end toolchain without action configs to cover the fallback.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Parse AR, LD, and STRIP as commands before resolving their executable. This keeps bare wrappers on PATH when a later argument contains a slash and preserves quoted arguments after the backend changes cwd. Exercise bare and execroot-relative commands in the native-sdist backend, including a real archiver invocation.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5743333367
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fall back to the public CcToolchainInfo tool paths when an action config is absent, but only when the path is a declared toolchain input. This keeps action-config tools authoritative and rejects fabricated legacy paths from action-only providers. Exercise the transitioned no_legacy_features toolchain with poisoned ambient tools and a real C/C++ compile, archive, and link.
|
Will this cover the cases outlined in #1131? Should those tests be added here? |
|
Will this fix the test case + bug demonstrated in #1299? |
|
#1131 is the py_cc_toolchain-registration issue (#1095), which #1187 already fixed. The existing pbs-cc-toolchain and cc-deps coverage checks that the selected PBS runtime and C payload match, exercises regular/free-threaded and ABI3, and compiles, links, and imports an extension including the exact header/runtime version check. This PR is the separate PEP-517 source-build path, so copying the compile-only tests from #1131 would not add coverage here. #1299 adds a nanobind consumer test for that already-fixed py_cc path; its e2e checks pass on Bazel 8 and 9. It is not a PEP-517 source-build reproduction, so this PR does not change its behavior. Keeping the nanobind fixture separate makes sense if we want that integration coverage without adding its dependencies here. |
|
The OpenAI snapshot bump caught a C++ source-build regression from this change: FastText links with the configured clang C driver and then fails import on the C++ ABI vtable. #1342 restores the declared clang++/g++ companion and adds an executable native-sdist regression that exercises std::string and RTTI. |
3 participants
Select the configured C++ action tools for native source builds and expose an EXECROOT anchor for toolchain-provided include and library paths, keeping them valid after the PEP 517 backend changes directory. Restore the native-dependency regression from #1233 and prevent action-only toolchains from publishing nonexistent legacy tool paths.