-
Notifications
You must be signed in to change notification settings - Fork 71
Add option to pass CMake cache file directly #1150
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
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 |
---|---|---|
|
@@ -281,6 +281,9 @@ def configure( | |
cache_config.update(cache_entries) | ||
|
||
self.config.init_cache(cache_config) | ||
cache_file_args = [] | ||
if self.settings.cmake.cache_file: | ||
cache_file_args.append(f"-C{self.settings.cmake.cache_file}") | ||
|
||
if sys.platform.startswith("darwin"): | ||
# Cross-compile support for macOS - respect ARCHFLAGS if set | ||
|
@@ -293,7 +296,11 @@ def configure( | |
|
||
self.config.configure( | ||
defines=cmake_defines, | ||
cmake_args=[*self.get_cmake_args(), *configure_args], | ||
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. Can we have a note for the record somewhere? The initial cache file generated by scikit-build-core will be added to the end of the argument list, or the beginning? Do we expect the cache files to be processed sequentially? The AIs I ask disagree on how CMake handles multiple 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. It would be after the first But on that note, should this be a single option or a list of cache files? 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. Oof. Well, I guess it wouldn't be too hard to accept a sequence, but I don't personally need it. I guess the processing would be similar to 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. I am open to either approach. It's just that this decision needs to be definite so that we don't change the schema afterwards. We ave cmake.args for more free-style args if needed 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. Since |
||
*cache_file_args, | ||
*self.get_cmake_args(), | ||
*configure_args, | ||
], | ||
) | ||
|
||
def build(self, build_args: Sequence[str]) -> None: | ||
|
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.
It would be good to test this, especially on Windows, to make sure that there isn't anything weird with path separators or escape characters. My Windows test machines are down right now, but I can probably help next week, if needed.
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.
Good point I will try to add it. It shouldn't be any issue because we use
Path
in other parts as well without special handling (build-dir
orsource-dir
), so that part should be fine (or we need to fix it in various places).What I want to be covered though is the order of these flags and that those are predictable.