[Build] Use CMAKE_ARGS directly, drop legacy QUADRANTS_CMAKE_ARGS#753
[Build] Use CMAKE_ARGS directly, drop legacy QUADRANTS_CMAKE_ARGS#753hughperkins wants to merge 6 commits into
Conversation
scikit-build-core's canonical CMake-args passthrough is CMAKE_ARGS, so the build.py tooling no longer needs a separate QUADRANTS_CMAKE_ARGS input that it mirrors into CMAKE_ARGS on writeback. Point CMakeArgsManager at CMAKE_ARGS for both input and output and drop the mirror. Because the manager now parses the same variable it writes, a user who exports CMAKE_ARGS before `build.py wheel`/`--shell`/`-w` has those defines absorbed and re-rendered rather than clobbered (fixes the review concern on #747), while the toolchain options the manager injects are still applied. Also switch the CI 2_build scripts to export CMAKE_ARGS. The Windows script was exporting the stale GSTAICHI_CMAKE_ARGS, which the manager never read, so Windows CI had been building with default options instead of the intended -DQD_WITH_VULKAN/CUDA/AMDGPU; this fixes that too. Docs updated accordingly.
hughperkins
force-pushed
the
hp/cmake-args
branch
from
3decea8 to
ddb2651
Compare
![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: ddb2651afc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -145,10 +143,10 @@ def __getitem__(self, name: str) -> Union[str, bool]: | |||
| return self.definitions[name] | |||
|
|
|||
|
|
|||
| cmake_args = CMakeArgsManager("QUADRANTS_CMAKE_ARGS") | |||
| cmake_args = CMakeArgsManager("CMAKE_ARGS") | |||
There was a problem hiding this comment.
After this line, the standard scikit-build CMAKE_ARGS passthrough is funneled through CMakeArgsManager.parse_initial_args(), whose DEF_RE only captures all-caps -D... entries and only the optional :BOOL type, and writeback() then replaces os.environ['CMAKE_ARGS'] with the rendered subset. CMake accepts typed cache entries such as -DCMAKE_BUILD_TYPE:STRING=Debug (cmake --help: -D <var>[:<type>]=<value>), and scikit-build-core treats CMAKE_ARGS as exact CMake args, so a user running CMAKE_ARGS='-GNinja -DCMAKE_BUILD_TYPE:STRING=Debug' ./build.py wheel silently loses those options before pip wheel sees them.
Useful? React with 👍 / 👎.
|
ok to merge |
CMakeArgsManager parsed CMAKE_ARGS with DEF_RE (only `-D<ALLCAPS>[:BOOL]=<value>`) and writeback() then overwrote CMAKE_ARGS with just the rendered subset, silently dropping anything else before scikit-build-core saw it: CMake generators (-GNinja), typed cache entries (-DCMAKE_BUILD_TYPE:STRING=Debug), lowercase-named or space-containing defines, and bare flags (-Wno-dev). Now that CMAKE_ARGS is the canonical passthrough (was QUADRANTS_CMAKE_ARGS), users reasonably expect it forwarded verbatim. parse_initial_args() now stashes the unparsed remainder (matched entries blanked out) and writeback() re-appends it after the rendered options. The two sets are disjoint, so there is no duplicate-cache-var ambiguity. Internal build flows are unaffected (they only emit all-caps untyped/:BOOL defines, which still round-trip).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce6d464401
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
|
|
||
| cmake_args = CMakeArgsManager("QUADRANTS_CMAKE_ARGS") | ||
| cmake_args = CMakeArgsManager("CMAKE_ARGS") |
There was a problem hiding this comment.
By pointing the manager at the public CMAKE_ARGS variable, valid CMake spellings such as -DQD_WITH_CUDA=0 or -DQD_WITH_CUDA:BOOL=FALSE now go through set() and hit assert ... must be bool because _VMAP only accepts exact ON/OFF. I checked cmake --help-command if, which documents 0, NO, FALSE, etc. and case-insensitive named constants as boolean values, so users disabling a backend with ordinary CMake boolean syntax can make build.py abort before the build starts.
Useful? React with 👍 / 👎.
| # entries (`-DCMAKE_BUILD_TYPE:STRING=Debug`), lowercase-named or space-containing defines -- | ||
| # must be preserved verbatim, else writeback() would silently drop it before scikit-build-core | ||
| # sees CMAKE_ARGS. Stash the unparsed remainder (matched entries blanked out) and re-emit it. | ||
| self.passthrough = " ".join(DEF_RE.sub(" ", args).split()) |
There was a problem hiding this comment.
This passthrough is computed by applying DEF_RE to the raw string, so a quoted define with spaces is split before it can be preserved. For example, CMAKE_ARGS='-DCMAKE_PREFIX_PATH="/tmp/a b" -DQD_WITH_CUDA=OFF' captures only "/tmp/a as the value and leaves b" in passthrough; writeback() then inserts rendered definitions between those fragments, so the quote spans across another -D argument and CMake receives malformed arguments. This is especially likely for dependency paths on Windows or SDK installs under directories with spaces.
Useful? React with 👍 / 👎.
2 participants
scikit-build-core's canonical CMake-args passthrough is CMAKE_ARGS, so the build.py tooling no longer needs a separate QUADRANTS_CMAKE_ARGS input that it mirrors into CMAKE_ARGS on writeback. Point CMakeArgsManager at CMAKE_ARGS for both input and output and drop the mirror.
Because the manager now parses the same variable it writes, a user who exports CMAKE_ARGS before
build.py wheel/--shell/-whas those defines absorbed and re-rendered rather than clobbered (fixes the review concern on #747), while the toolchain options the manager injects are still applied.Also switch the CI 2_build scripts to export CMAKE_ARGS. The Windows script was exporting the stale GSTAICHI_CMAKE_ARGS, which the manager never read, so Windows CI had been building with default options instead of the intended -DQD_WITH_VULKAN/CUDA/AMDGPU; this fixes that too. Docs updated accordingly.
Issue: #
Brief Summary
copilot:summary
Walkthrough
copilot:walkthrough