Skip to content

CMake improvements#169

Merged
rileyjmurray merged 13 commits into
mainfrom
cmake-improvements
May 25, 2026
Merged

CMake improvements#169
rileyjmurray merged 13 commits into
mainfrom
cmake-improvements

Conversation

@rileyjmurray

Copy link
Copy Markdown
Contributor

This PR brought to you by back-and-forth with a robot.

Tightens the install machinery, the exported package, and assorted build-time UX so that both downstream find_package(RandBLAS) consumers and RandBLAS' own developers get a cleaner, more modern, more predictable experience. Each change stands alone; the commit history reflects that grouping.

Consumer-visible behavior

Before this PR, a downstream project that called find_package(RandBLAS) could expect:

  • -Wall, -Wextra, -Wno-comment silently imposed on its own code — plus -fsanitize=address (on both compile and link lines) if RandBLAS happened to be built with SANITIZE_ADDRESS=ON. Consumers got ASan'd without opting in, often producing confusing link failures because libasan wasn't on the link line.
  • No declaration of RandBLAS' C++20 requirement on the exported target, so the consumer compiled against whatever its own default standard was.
  • A reliance on blaspp's find_dependency(OpenMP) to incidentally pull OpenMP into scope, despite RandBLAS' own exported target referencing OpenMP::OpenMP_CXX directly.
  • A bare-named Random123 target leaking into the consumer's global namespace.
  • CMake config files installed directly under lib/cmake/, requiring consumers to set -DRandBLAS_DIR=<prefix>/lib/cmake explicitly because the layout didn't match CMake's package-auto-discovery search.

After this PR:

  • The exported RandBLAS target carries no INTERFACE_COMPILE_OPTIONS or INTERFACE_LINK_OPTIONS. Build-time-only flags (warnings, sanitizers) are wrapped in $<BUILD_INTERFACE:...> so they apply in-tree and disappear from the installed export.
  • The target advertises cxx_std_20 as a compile feature, so consumers automatically get -std=gnu++20 (or equivalent) without having to set CMAKE_CXX_STANDARD themselves.
  • RandBLASConfig.cmake calls find_dependency(OpenMP COMPONENTS CXX) directly when RandBLAS was built with OpenMP. No more incidental dependence on blaspp's behavior.
  • Random123 is exposed to consumers as the namespaced IMPORTED target Random123::Random123, recreated on the consumer side by RandBLAS' installed FindRandom123.cmake. The bare Random123 global target is gone.
  • CMake config files now install to lib/cmake/RandBLAS/, the canonical layout. find_package(RandBLAS) resolves via -DCMAKE_PREFIX_PATH=<prefix> alone; the RandBLAS_DIR workaround is no longer needed.
  • The CMake floor is bumped to 3.12 across the tree (top-level, examples, INSTALL.md snippets). This matches the actual feature set we depend on — target_compile_features(... cxx_std_20) was added in CMake 3.12 — and consumers of our cxx_std_20 feature need at least that.

Build-time UX

  • New BUILD_TESTS CMake option, default ON. cmake -DBUILD_TESTS=OFF (or =no, =false, etc. — case-insensitive per CMake's own keyword handling) skips configuring the test/ subdirectory entirely. Useful for downstream packaging or quick configure-only sanity checks.
  • The examples build now defaults CMAKE_BUILD_TYPE to Release if none was supplied, instead of unconditionally setting CMAKE_CXX_FLAGS="-O1". The latter both overrode -O3 from Release mode and stomped any flags the user explicitly passed. The new pattern preserves user overrides and produces properly optimized binaries by default.
  • The stat_tests binary no longer depends on its current working directory at runtime. Previously the KAT vector file was copied to three destinations in the build tree, all of them wrong for the common dev workflow of invoking the binary directly. The file is now read via a compile-time-baked absolute path, so the test works whether invoked through ctest, cd bin && ./stat_tests, ./bin/stat_tests from the build root, or via an absolute path from anywhere.

Cleanups

  • Deleted CMake/blaspp.cmake and CMake/Random123.cmake — dead modules that defined RandBLAS_blaspp / RandBLAS_Random123 INTERFACE wrappers and were never include()'d.
  • Removed stale -D __APPLE__ install advice from INSTALL.md and the project's CLAUDE.md; the related source-side workaround was already gone.
  • Dropped the deprecated EXPORT_LINK_INTERFACE_LIBRARIES argument from the install(EXPORT) call. It's only meaningful for CMake older than 2.8.12 and we require 3.12.
  • Removed a dead list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/CMake") line from examples/CMakeLists.txt — it pointed at a directory that doesn't exist, and the file uses no custom include() calls anyway.
  • Fixed an arg-order typo in CMake/rb_build_options.cmake: option(BUILD_SHARED_LIBS OFF "Configure to build shared or static libraries") had the value and help-text positions swapped. After the fix, the cache entry is properly typed (BUILD_SHARED_LIBS:BOOL=OFF) with the intended help text.

Verification

For each individual change:

  • The find_package(RandBLAS) install was inspected to confirm the corresponding fix in the generated RandBLAS.cmake / RandBLASConfig.cmake files.
  • A minimal external CMake consumer was built against the installed prefix to confirm consumer-visible behavior matched the expected before / after.
  • The full in-tree test suite (ctest) was re-run after each change. 429/429 tests pass.
  • For the BUILD_TESTS option, six values were exercised: default, OFF, yes, on, ON, True — all behave correctly via CMake's case-insensitive keyword coercion.
  • For the stat_tests cwd-independence change, the binary was invoked from four different working directories (bin/, build root, /tmp, and ctest's default cwd). All four pass.

Potential follow-up work

  • Pin and cache CI dependencies. Today the GitHub Actions workflows git clone https://github.com/icl-utk-edu/blaspp.git (and Random123) at main on every run. This makes CI non-reproducible against upstream churn AND wastes 1-3 minutes of build time per OS per PR. The right pattern is to pin each dep to a specific ref in the workflow, then use actions/cache keyed on that ref. The same pattern would apply to fast_matrix_market in examples/CMakeLists.txt, which today is FetchContent_Declare'd at main.
  • Harden MKL sparse BLAS detection. Not exercised here because the development environment is Apple Silicon (no MKL). The current detection logic in CMake/MKL_sparse.cmake is worth reviewing on a Linux + MKL host.
  • Add CMakePresets.json for one-line reproducible local build configurations (Release, Debug, Debug+ASan, etc.). Lets developers avoid memorizing the cmake command line that's currently spread across INSTALL.md.
  • Replace the version-regex in CMake/rb_version.cmake with explicit git describe / git rev-parse invocations. The current approach scrapes a file with a regex; the git-based approach is more robust and self-documenting.
  • Push a namespaced blaspp::blaspp target upstream. blaspp's installed Config exports a bare-named blaspp target, which is why our own target_link_libraries(... blaspp Random123::Random123 ...) is asymmetric. The right fix is an upstream PR adding add_library(blaspp::blaspp ALIAS blaspp) to blaspp's install, after which we can use the namespaced name here too.
  • Revisit -Wno-comment. The workaround was inherited from earlier code. If the underlying comment-related diagnostic in the headers no longer fires under current compilers, this flag can be deleted.
  • Reconsider how SANITIZE_ADDRESS is plumbed. A more idiomatic alternative is to let users set -fsanitize=address via CMAKE_CXX_FLAGS (or a preset) and remove the bespoke option entirely. This is a behavior change worth its own discussion.

@rileyjmurray

Copy link
Copy Markdown
Contributor Author

@mmelnich please review these changes for situational awareness. You'll likely need to update your RandLAPACK build script once you update it to use this or newer RandBLAS.

@mmelnich

Copy link
Copy Markdown
Contributor

@rileyjmurray Looks good; will update RandLAPACK once this is merged.

@rileyjmurray rileyjmurray merged commit bdec958 into main May 25, 2026
5 checks passed
@rileyjmurray rileyjmurray deleted the cmake-improvements branch May 25, 2026 16:39
@rileyjmurray rileyjmurray mentioned this pull request May 25, 2026
rileyjmurray added a commit that referenced this pull request May 25, 2026
This PR brought to you by a robot.

Replaces the four legacy CI workflows with a single matrix workflow plus
dedicated workflows for ThreadSanitizer, examples, downstream-consumer
smoke, and docs.

Adds a Docker dev image for running TSan locally on
Apple Silicon. Raises the documented minimum compiler floor.

### New workflows

- `core.yml` — single matrix replacing four legacy files. 11 cells:
Linux gcc/clang × OpenBLAS/MKL × Release/Debug × {none, ASan, UBSan};
macOS 14 Apple Clang (serial) + Homebrew LLVM (OpenMP) × Accelerate ×
{none, ASan}; macOS 15 Homebrew LLVM tracking the latest runner.
- *All OpenMP cells run `ctest` twice — `OMP_NUM_THREADS=1` and `=4` —
so the thread-independence invariant is actually exercised, not just
compiled.*
- *OpenBLAS and MKL are tested in separate Linux builds*, so the
`RandBLAS_HAS_MKL` codepaths get real coverage instead of "whichever
blaspp happened to pick."
- `thread-sanitizer.yml` — clang + libomp + `-fsanitize=thread` on
Linux. *Defends the thread-independence invariant on the RNG state
machinery that ASan can't catch.*
- `examples.yml` — builds `examples/` against an installed RandBLAS
(with `lapackpp` from the composite action). Catches downstream API
drift in real consumer code.
- `downstream-consumer.yml` + `test/downstream/` —
`find_package(RandBLAS)` from a tiny isolated consumer. Guards the
install/export rules in `RandBLAS/CMakeLists.txt`.
- `docs.yml` — `doxygen + sphinx-build`. *Shadow build for now (no `-W`)
— follow-up after the docstring warning backlog is cleared.*
- `.github/actions/setup-randblas-deps/` — composite action installing
system deps and building blaspp + Random123 (+ optionally lapackpp),
cached on upstream commit SHA. Eliminates the heavy copy-paste across
the old workflows.

### Removed

- The four legacy workflows (`core-linux.yaml`, `core-macos.yml`,
`openmp-macos.yaml`, `openmp-macos-15.yaml`). *The last two were
converging to the same job once `macos-latest` rolls to 15.*

### Local TSan tooling

- `docker/tsan/Dockerfile` + `run.sh` — Linux container with clang +
libomp + sanitizer runtimes + `llvm-symbolizer` + baked
blaspp/Random123. *Necessary because TSan can't run natively on Apple
Silicon (shadow-memory mapping fails at startup before TSan can even
print its banner).* Usage:
- `./docker/tsan/run.sh build-and-test` — full TSan ctest pass in one
shot.
  - `./docker/tsan/run.sh` — interactive shell for iteration.
- `./docker/tsan/run.sh rebuild-image` — refresh blaspp / Random123
upstream.

### Hygiene

- `actions/checkout@v4` everywhere (three of four legacy files were on
deprecated `@v2`).
- `concurrency:` group with `cancel-in-progress: true` so superseded PR
runs auto-cancel.
- `push: branches: [main]` trigger so main's CI status reflects current
state, not just the merge-time check.
- `make -j$(nproc)` / `sysctl -n hw.logicalcpu` instead of `-j2`.
- blaspp / lapackpp / Random123 builds cached, keyed on upstream commit
SHA.

### Doc updates

- `CLAUDE.md` and `INSTALL.md`: documented minimum compiler raised gcc-9
→ gcc-13 to match `ubuntu-latest`. Old gcc-8.5 / `-fconcepts` workaround
note removed.

### Explicitly not in this PR

- *Windows / MSVC* — out of scope.
- *gcc-9 cell* — dropped in favor of raising the documented floor.
- *`-W` on docs* — deferred until the docstring warning baseline is
clean.
- *GCC-on-macOS cell* — easy follow-up if needed.

## Verification

- All 11 `core.yml` cells green on this branch.
- `thread-sanitizer.yml`: locally green via `docker/tsan/run.sh
build-and-test`. Instrumentation verified end-to-end by *planting a
deliberate harmless race in `dense_skops.hh` and confirming TSan caught
it*, then reverting — so the clean baseline isn't a
green-because-everything-is-suppressed.
- `examples.yml` and `downstream-consumer.yml` re-pointed at the new
CMake config layout from #169 (`<prefix>/lib/cmake/RandBLAS/`) and
switched to `-DCMAKE_PREFIX_PATH=<prefix>` per the new INSTALL.md
recommendation, so the same class of layout-change breakage can't recur.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants