-
Notifications
You must be signed in to change notification settings - Fork 26
[Build] Use CMAKE_ARGS directly, drop legacy QUADRANTS_CMAKE_ARGS #753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6318f3e
ddb2651
cb2b61f
ce6d464
734a720
5101579
f099c18
5441c30
db44ae3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,8 @@ def __init__(self, environ_name): | |
| self.option_definitions = { | ||
| "CMAKE_EXPORT_COMPILE_COMMANDS": ("Generate compile_commands.json", False, ""), | ||
| } | ||
| # CMAKE_ARGS entries we don't parse into definitions, kept verbatim for writeback(). | ||
| self.passthrough = "" | ||
|
|
||
| self.finalized = False | ||
|
|
||
|
|
@@ -42,6 +44,12 @@ def collect_options(self, *files: str) -> None: | |
|
|
||
| def parse_initial_args(self) -> None: | ||
| args = os.environ.get(self.environ_name, "") | ||
| # DEF_RE only understands `-D<ALLCAPS>[:BOOL]=<value>` entries (the options we manage and | ||
| # render below). Everything else in CMAKE_ARGS -- CMake generators (`-GNinja`), typed cache | ||
| # 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()) | ||
| for name, value in DEF_RE.findall(args): | ||
| self.set(name, value) | ||
|
|
||
|
|
@@ -129,13 +137,17 @@ def print_summary(self, rendered) -> None: | |
| def writeback(self) -> None: | ||
| rendered = self.render() | ||
| self.print_summary(rendered) | ||
| value = " ".join([v for _, v, _ in rendered]) | ||
| # Rendered options first, then the verbatim passthrough captured in parse_initial_args. The | ||
| # two sets are disjoint (parsed entries are blanked out of passthrough), so there is no | ||
| # duplicate-cache-var ambiguity for CMake to resolve. | ||
| parts = [v for _, v, _ in rendered] | ||
| if self.passthrough: | ||
| parts.append(self.passthrough) | ||
| value = " ".join(parts) | ||
| # CMAKE_ARGS is scikit-build-core's standard CMake-args passthrough, and it is also what we parse on input, so | ||
| # writing it back makes the environment exported by `build.py wheel`, `-w`, and `--shell` directly usable by the | ||
| # build with no further bridging. | ||
| os.environ[self.environ_name] = value | ||
| # scikit-build-core reads CMake args from CMAKE_ARGS, not the legacy QUADRANTS_CMAKE_ARGS. | ||
| # Mirror the rendered args there so the environment exported by `build.py wheel`, `-w`, and | ||
| # `--shell` is directly usable -- no manual `export CMAKE_ARGS="$QUADRANTS_CMAKE_ARGS"` step. | ||
| if self.environ_name == "QUADRANTS_CMAKE_ARGS": | ||
| os.environ["CMAKE_ARGS"] = value | ||
| self.finalized = True | ||
|
|
||
| def __setitem__(self, name: str, value: Union[str, bool]) -> None: | ||
|
|
@@ -145,10 +157,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
After this line, the standard scikit-build Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
By pointing the manager at the public Useful? React with 👍 / 👎. |
||
|
|
||
|
|
||
| @banner("Parsing QUADRANTS_CMAKE_ARGS") | ||
| @banner("Parsing CMAKE_ARGS") | ||
| def _init_cmake_args(): | ||
| cmake_args.collect_options("CMakeLists.txt", *glob.glob("cmake/*.cmake")) | ||
| cmake_args.parse_initial_args() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This passthrough is computed by applying
DEF_REto 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/aas the value and leavesb"inpassthrough;writeback()then inserts rendered definitions between those fragments, so the quote spans across another-Dargument and CMake receives malformed arguments. This is especially likely for dependency paths on Windows or SDK installs under directories with spaces.Useful? React with 👍 / 👎.