-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add find_package overrides that also work in CONFIG mode (#498) #604
Add find_package overrides that also work in CONFIG mode (#498) #604
Conversation
b34fff7
to
121410c
Compare
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.
Thanks a lot for the PR and sorry for taking long to review it! It looks great on first glance, I have one small suggestion for readability. Also I'm not sure why the CI pipelines are not running, do you need to enable GitHub actions for your fork?
121410c
to
b9499c0
Compare
No worries, thanks for CPM. |
Yep this time it showed the button, thanks for update! Looks like we need to update the tests to support the different behavior based on the CMake version and also run |
b9499c0
to
8497a75
Compare
8497a75
to
8ff2d08
Compare
I think I've fixed the issues (patstew#1). I've also added a few checks to the |
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.
Looks great, thanks for updating this feature and the according documentation and test changes!
Was there was a reason for the find_package to be disabled previously?
Probably an oversight, good catch!
Changes released in |
CPM currently adds a FindFoo.cmake to the module path to override subsequent find_package calls. This makes sure that works when the find_package call is in CONFIG mode as well. Some of my dependencies use this in their CMakeLists.txt. This is equivalent to the OVERRIDE_FIND_PACKAGE argument to FetchContent, we could add that option to CPMAddPackage, but it seems like this is the intended default behaviour of CPM?
The include(....-extra.cmake) bits are something FetchContent does that's also useful here. They should maybe be added to the Find*.cmake version too?
Fixes #498