add optional name for header set to install #10
Conversation
steve-downey
requested review from
JeffGarland,
ednolan,
neatudarius,
rishyak and
wusatosi
as code owners
| # The prefix `<PREFIX>` is the uppercased name of the library with dots | ||
| # replaced by underscores. | ||
| # | ||
| set(options "") |
There was a problem hiding this comment.
I think this violates our own guidelines to not set single use variables -- shouldn't we just pass this in at line #44 or am I misunderstanding (entirely possible) the cmake here. And ARGN is the only external thing here.
There was a problem hiding this comment.
oh maybe these are defaults if not set by cookie cutter or something? obviously I don't understand what this is doing so a comment might be helpful
There was a problem hiding this comment.
Stringly typed confusables. There has to be something in those positions, and without names it's a bunch of empty strings?
As an alternative, though, I could add end of line comments. Or just inline the strings.
There was a problem hiding this comment.
Stringly typed confusables. There has to be something in those positions, and without names it's a bunch of empty strings?
As an alternative, though, I could add end of line comments. Or just inline the strings.
| NAMES "${package_name}-config.cmake.in" | ||
| PATHS "${CMAKE_CURRENT_SOURCE_DIR}" | ||
| PATHS | ||
| "${CMAKE_CURRENT_SOURCE_DIR}" |
There was a problem hiding this comment.
I recently had to fix an issue with this in take_before repo after repo had been initialized from examplar -- which put the cmake.in file into the src/ subtree. take_before was converted to an interface (aka header only) library so src/ was removed. config.cmake.in got moved to the root directory since the packaging cmake got moved there and these other options weren't available. Should we consider standardizing the location under /cmake directory?
There was a problem hiding this comment.
I think that's the best place for it, and that's what the two new search paths do. Although it's possible only the PROJECT one is really needed? That handles the case where the project is vendored in and added by add_subdirectory.
There was a problem hiding this comment.
I think that's the best place for it, and that's what the two new search paths do. Although it's possible only the PROJECT one is really needed? That handles the case where the project is vendored in and added by add_subdirectory.
|
I think this pull request is obsoleted by the following changes: The new standard is that your |
That's good, but I don't think find_package is the correct construct there -- it should just be an include. Like this one: https://github.com/bemanproject/take_before/blob/main/CMakeLists.txt#L42-L43 |
4 participants
Add an optional name for the header set.
Add more search paths for, e.g., "beman.optional-config.cmake.in" than just infra/cmake, like the top-level or project level cmake directories.
The infra directory is shared infrastructure, and the project "in" file isn't.