Add option to pass CMake cache file directly#1150
Add option to pass CMake cache file directly#1150LecrisUT wants to merge 2 commits intoscikit-build:mainfrom
Conversation
Sorry I missed the call this morning. Thanks for the help! |
| 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}") |
There was a problem hiding this comment.
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.
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 or source-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.
| self.config.configure( | ||
| defines=cmake_defines, | ||
| cmake_args=[*self.get_cmake_args(), *configure_args], | ||
| cmake_args=[ |
There was a problem hiding this comment.
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 -C arguments.
There was a problem hiding this comment.
It would be after the first -C that we add, but otherwise the order is a bit ill defined, I'll try to do some troubleshooting to see how -D flags impact it and if we need to reorder it.
But on that note, should this be a single option or a list of cache files?
There was a problem hiding this comment.
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 cmake.define, so it wouldn't be unusual or surprising for users or developers.
There was a problem hiding this comment.
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.
Since cmake accepts multiple -C arguments, it would seem more correct to accept a list. I can't think of a good reason not to accept a list other than the effort and extra burden of support (documentation, test coverage, ...)
|
What's wrong with passing |
Mainly 2 usages:
|
For the record, I have added It might be that the integration tests for this issue are more important than the feature, now that https://github.com/orgs/scikit-build/discussions/1154#top is on record (even if not particularly google-able yet). |
3 participants
@eirrgang sorry it took so long to get to this, but this should give you a more direct way to hook your cache-file as needed