Draft: CMake improvements#248
Open
caitlinross wants to merge 15 commits into
Open
Conversation
- convert global add_definitions(-DUSE_*) to target_compile_definitions on codes - drop redundant CMAKE_C_FLAGS include appends (already covered by target_include_directories) - executables/tests inherit includes + libs transitively by linking codes
The various dependencies had some inconsistencies in how they were handled. Some if found in PATH, could not be turned off. Now all have three possible states: AUTO, ON, OFF. All are AUTO by default meaning that we will attempt to find them and if found, build with them. If a dependency is not found, there is no error or warning, we just build without it. If a dependency is set to OFF, it won't be looked for at all, and if a dependency is set to ON and not found, an error will be given. Also changed naming from USE_* to CODES_USE_* to prevent any collisions with other projects that may use CODES.
Optional-dependency state (DUMPI, ONLINE, SWM, UNION, RECORDER, TORCH,
ZEROMQ, DARSHAN) was carried as bare -DUSE_<X> defines added PUBLIC to the
codes target, so every one of those names leaked onto the command line of
everything that links codes. That approach had two real problems:
- The names are generic and unnamespaced. USE_ONLINE / USE_TORCH and the
like can collide with whatever a downstream project defines, and
USE_ONLINE was already baked into an installed public header
(codes-workload.h) -- so the leak was part of our public surface.
- #ifdef tests existence, not value. A stray -DUSE_X=0 would *enable* the
feature, and any source that forgot the define or typo'd it silently
compiled the feature off, with no diagnostic at all.
Instead, generate a single namespaced header (codes_config.h) defining
CODES_HAVE_<X> to 0/1 for each optional subsystem, install it alongside the
public headers, and query it with `#if CODES_HAVE_<X>`. Build CODES's own
targets with -Wundef.
This is strictly better than the old scheme: feature availability is
namespaced (no clashes), it lives in one installed header consumers can
actually query instead of an invisible command-line contract, and nothing
leaks onto consumer compile lines. The `#if` + -Wundef combination turns the
classic preprocessor foot-guns -- forgotten include, typo'd macro, -DFOO=0
-- into compile-time warnings rather than a silently mis-built feature.
CODES installed a static library and headers but no CMake package, so a
downstream model had no way to find_package(codes) -- it had to hard-code
CODES's include and library paths (and ROSS's, and MPI's) by hand, or vendor
the tree. That is exactly the fragile, manual coupling the rest of this
effort has been removing everywhere else.
This adds a namespaced install(EXPORT) that generates codesConfig.cmake and a
codes::codes imported target, mirroring how ROSS already exposes ROSS::ROSS
(and how CODES now consumes ROSS). A consumer does
find_package(codes REQUIRED)
target_link_libraries(mymodel PRIVATE codes::codes)
and inherits CODES's public include dirs plus its ROSS / MPI usage
requirements transitively -- no bespoke CODES_INCLUDE_DIRS / CODES_LIBRARIES
variables, no knowing where ROSS lives. The config re-resolves the
dependencies CODES links (find_dependency for ROSS and MPI C/CXX).
To keep the exported target relocatable, the library's public include
interface moves to BUILD_INTERFACE / INSTALL_INTERFACE generator
expressions: in-tree builds see the source/binary dirs, installed consumers
get <prefix>/include and <prefix>/include/codes, and no build-machine path
leaks into the installed codesTargets.cmake. Verified with a standalone
find_package(codes) consumer that links codes::codes and compiles a CODES
header.
caitlinross
force-pushed
the
cmake-improvements
branch
2 times, most recently
from
d9a58c3 to
5838ec3
Compare
caitlinross
force-pushed
the
cmake-improvements
branch
from
5838ec3 to
6cac1c3
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.