More CMake Polish#208
Merged
caitlinross merged 4 commits intoJun 16, 2026
Merged
Conversation
ROSS_OPTION_LIST was a string macro carrying a stringified list of
-DROSS_QUEUE_${QUEUE} / -DROSS_RAND_${RAND} / etc. flags into the
binary; tw-opts.c::show_options() parsed it at runtime to pretty-print
"ROSS Kernel Build Options" to stderr. Commit 085229d (2014) deleted
show_options() and migrated the same selections to config.h's #define
ROSS_QUEUE_ROSS_QUEUE_${QUEUE} family, which is what backs the #ifdef
ROSS_QUEUE_splay / ROSS_NETWORK_mpi / ROSS_GVT_mpi_allreduce sites in
ross-types.h / ross-network.h / ross-clock.h today. The CMake-side
SET(OPTIONS ...) producer was commented out then, and the
SET(CMAKE_C_FLAGS ... -DROSS_OPTION_LIST=...) consumer kept defining a
macro nothing reads. Eleven years of dead code; delete the whole
mechanism.
The recursive FILE(GLOB_RECURSE . FOLLOW_SYMLINKS */CMakeLists.txt) in models/CMakeLists.txt had two real hazards: it walked into build/ install/test subdirectories if any of those grew CMakeLists.txt files, and FOLLOW_SYMLINKS combined with recursion was a footgun for any nested build dir reached through a symlink. Narrow it to a flat top-level FILE(GLOB LIST_DIRECTORIES TRUE ...) filtered by IS_DIRECTORY + EXISTS .../CMakeLists.txt. CMAKE_POLICY(SET CMP0009 NEW) is deleted along with it — CMP0009 only affects GLOB_RECURSE + FOLLOW_SYMLINKS, so it has no remaining caller. The glob was also load-bearing for an older workflow where users develop a model in a separate repo, symlink it into ross/models/, and rely on the glob to pick it up. A recent PR made find_package(ROSS) work, so external models can now be standalone CMake projects. Retire the symlink path from the documented surface: models/README.md is rewritten around a 10-line find_package(ROSS) worked example, with a brief "if you have one, migrate" note for current users. The flat glob still picks up symlinked entries, so existing users are not silently broken — but an IS_SYMLINK guard inside the foreach fires message(DEPRECATION) for each symlinked model, surfacing the migration prompt at configure time. The whole block (glob, IS_SYMLINK guard, deprecation comment) goes away in a future PR once the workflow has been deprecated long enough.
ROSS_TEST_SCHEDULERS / ROSS_TEST_INSTRUMENTATION in
models/CMakeLists.txt hardcoded "mpirun -np 2 ./${target_name}" for the
MPI-launched tests, which only worked on systems where (a) the launcher
binary was named mpirun, (b) the proc-count flag was -np, and (c)
ctest's CWD was the test binary's directory at the moment of
invocation. Switch to the find_package(MPI)-provided variables —
${MPIEXEC_EXECUTABLE} ${MPIEXEC_NUMPROC_FLAG} 2 ${MPIEXEC_PREFLAGS}
$<TARGET_FILE:${target_name}> <args> ${MPIEXEC_POSTFLAGS} — so the
tests run under whatever launcher CMake detected (mpiexec on macOS
Homebrew MPICH, mpirun on OpenMPI, srun/jsrun on HPC sites that pass
prefix args through MPIEXEC_PREFLAGS as a CMake list).
Required converting every ADD_TEST(name cmd args) to the keyword
add_test(NAME ... COMMAND ...) form so the $<TARGET_FILE:...>
generator expression expands. Single-rank tests (Sequential, OptDebug,
RollbackCheck, _INST_Seq) intentionally skip the launcher wrapper —
--synch=1, --synch=4, and --synch=6 have a single-rank contract that
would break under mpiexec. Comment block at the top of the file
documents the conventions for future maintainers.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #208 +/- ##
=======================================
Coverage 48.75% 48.75%
=======================================
Files 41 41
Lines 4816 4816
Branches 860 860
=======================================
Hits 2348 2348
Misses 2463 2463
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
Hmm so the |
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.
Three small, independent CMake cleanups.
There was a dead ROSS_OPTION_LIST mechanism in the CMake. It seems the relevant code was deleted in 2014 in commit 085229d, so I got rid of the related CMake code.
I think the symlink workflow for building external ROSS models is not the typical approach. Since previous PRs have modernized the ROSS's CMake and correctly packages ROSS now, we should encourage a more typical CMake workflow. Right now the symlink approach still works, but there is a deprecation message when a symlinked model is built. The models README was updated to document this and show a simple CMake example.
Improve the model test helpers when using MPI.
mpirun -np 2was hardcoded, now we use the variables provided byfind_package(MPI)so you should be able to run tests correctly on any system.Checklist
-Walland-WextraDocumentation/dev/, unless the change is invisible to anyone outside the PR (test refactors, internal renames, comment-only tweaks)