Convert exemplar into an INTERFACE library#262
Conversation
ednolan
requested review from
bretbrownjr,
camio,
dietmarkuehl,
neatudarius,
steve-downey and
wusatosi
as code owners
| FILE_SET HEADERS | ||
| BASE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/include" | ||
| FILES | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/include/beman/exemplar/identity.hpp" |
There was a problem hiding this comment.
It's interesting how the header list moves to the top-level directory when it's a header-only library. This has two drawbacks:
- A header-only -> with-source-file transition is more dramatic.
- For a repository with multiple libraries, the top-level CMake file would grow quite large and we lose the "one CMakeLists.txt file per library" separation of concerns.
What are your thoughts on peeing the src/beman/exemplar directory and having it contain only a CMakeLists.txt that builds the target?
There was a problem hiding this comment.
It shouldn't be necessary to have the list of files in the root, but I have found that creating the library and naming the header set at the root makes it more local.
https://github.com/steve-downey/optional/blob/main/CMakeLists.txt#L23-L28
and
https://github.com/steve-downey/optional/blob/main/include/beman/optional/CMakeLists.txt#L4-L15
There was a problem hiding this comment.
It shouldn't be necessary to have the list of files in the root, but I have found that creating the library and naming the header set at the root makes it more local.
The latest version of this PR uses this approach.
|
Updated the TODOs. I might help out. Got the permission from @ednolan to push directly on this branch. |
wusatosi
force-pushed
the
enolan_interface1
branch
3 times, most recently
from
8ed607f to
36dd20f
Compare
ednolan
force-pushed
the
enolan_interface1
branch
from
36dd20f to
ffff523
Compare
ednolan
changed the title
|
It would be good to move this forward asap. I think you're going to need to rectify with main -- in particular this PR which got merged: |
|
|
||
| include(CTest) | ||
|
|
||
| find_package(beman-install-library REQUIRED) |
There was a problem hiding this comment.
See my comment about merging from main -- this should be include not find_package
There was a problem hiding this comment.
I agree we should move forward with this ASAP...
This is currently blocking infra upstream to inplace vector.
The trouble I had when I was working on it is we need to come up with a way to test the template effectively, preferably create a CI job that just runs something like make --preset llvm-debug for most possible cookie cutter variants. Otherwise I see challenges on maintaining more cookie cutter parameters going forward.
Edit: oh such test is included, nice job @ednolan
This comment was marked as outdated.
This comment was marked as outdated.
wusatosi
left a comment
wusatosi
left a comment
There was a problem hiding this comment.
Thanks for picking up the work Eddie.
This commit eliminates the src/ directory, moving the contents of src/beman/exemplar/CMakeLists.txt into the top level CMakeLists.txt (except for the header FILE_SET, which now lives in include/beman/exemplar/CMakeLists.txt), removing identity.cpp, and changing beman.exemplar from STATIC/SHARED into INTERFACE. The cookiecutter template now allows the user to specify whether they want to stamp out an "interface" or "static" library, where the "static" option is similar to exemplar's previous status quo. To ensure that cookiecutter's "static" option remains valid, we add a new CI test that smoke tests that building, testing, and installing it completes without error.
ednolan
force-pushed
the
enolan_interface1
branch
from
ffff523 to
bae3375
Compare
6 participants
This commit eliminates the src/ directory, moving the contents of src/beman/exemplar/CMakeLists.txt into the top level CMakeLists.txt (except for the header FILE_SET, which now lives in include/beman/exemplar/CMakeLists.txt), removing identity.cpp, and changing beman.exemplar from STATIC/SHARED into INTERFACE.
The cookiecutter template now allows the user to specify whether they want to stamp out an "interface" or "static" library, where the "static" option is similar to exemplar's previous status quo.
To ensure that cookiecutter's "static" option remains valid, we add a new CI test that smoke tests that building, testing, and installing it completes without error.
Closes bemanproject/beman#175