-
Notifications
You must be signed in to change notification settings - Fork 31
WIP Draft: Convert exemplar into an INTERFACE library #262
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?
Conversation
This commit eliminates the src/ directory, moving beman.examplar-config.cmake.in into the top level directory, moving the contents of src/beman/exemplar/CMakeLists.txt into the top level CMakeLists.txt, removing identity.cpp, and changing beman.exemplar from STATIC/SHARED into INTERFACE. This commit is currently just intended to be a jumping-off point for discussion. What I would like to do if we are okay with this direction is to preserve the option for users to create STATIC/SHARED libraries by configuring cookiecutter differently. That feature is not present yet here.
|
I opened a thread on Discourse here: https://discourse.bemanproject.org/t/converting-exemplar-to-a-cmake-interface-type-library/527 |
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
Updated the TODOs. I might help out. Got the permission from @ednolan to push directly on this branch. |
3d6ea39 to
d0eac83
Compare
d0eac83 to
e0141a1
Compare
8ed607f to
36dd20f
Compare
This commit eliminates the src/ directory, moving
beman.examplar-config.cmake.in into the top level directory, moving the contents of src/beman/exemplar/CMakeLists.txt into the top level CMakeLists.txt, removing identity.cpp, and changing beman.exemplar from STATIC/SHARED into INTERFACE.
This commit is currently just intended to be a jumping-off point for discussion. What I would like to do if we are okay with this direction is to preserve the option for users to create STATIC/SHARED libraries by configuring cookiecutter differently. That feature is not present yet here.
Edit(River):
closes bemanproject/beman#175
Copy pasted from bemanproject/beman#175 :
Remaining todos: