Skip to content

[newchem-cpp] Assorted CMake Improvements#505

Open
mabruzzo wants to merge 8 commits intograckle-project:newchem-cppfrom
mabruzzo:ncc/improve-cmake
Open

[newchem-cpp] Assorted CMake Improvements#505
mabruzzo wants to merge 8 commits intograckle-project:newchem-cppfrom
mabruzzo:ncc/improve-cmake

Conversation

@mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Feb 9, 2026

This PR makes 3 related adjustments to the CMake build-system.

These changes were all made in anticipation to an in-progress pull-request that adds basic benchmarks. The benchmarking executable requires google-benchmark as optional dependency. Just like with the googletest dependency, the when we configure a building requiring google-benchmark, the build-system will automatically search for a preinstalled version of google-benchmark, and if it can't be found the source code will automatically be downloaded and compiled as part of the build.

This PR addresses some stuff that came up in the course of that work.


This PR essentially makes 3 changes:

  1. it bumps the minimum required CMake version from 3.16 to 3.22
    • version 3.22 was released in 2021
    • my feeling has always been that we should always set this based on the version you can download with apt for the oldest LTS Ubuntu release that still receives general updates
    • CMake version 3.16 corresponded to Ubuntu 20.04 (stopped receiving updates last year)
    • CMake version 3.22 corresponds to Ubuntu 22.04
    • In practice, its actually very easy to download a newer CMake version. Kitware (the CMake developers) distribute precompiled binaries. Or you could even download a version through pip.
    • We needed to update to at least 3.18 so that I could properly forward arguments between CMake commands (needed for change 3)
  2. It turns out that when I originally added clang-tidy support (recall that it's a C++ linter), I made an oversight
    • with the way that clang-tidy gets set up, it turns out that we will run it on all dependencies that get built as an embedded part of the Grackle build (this also applies to any other linters that we use in the future)
    • I didn't notice this before because clang-tidy doesn't complain about the source code of googletest. This is actually a bit of anomaly -- presumably because of the widespread popularity of googletest.
    • More generally, we would static-linters to raise warnings for dependencies. For example some linters, like clang-tidy, can be configured with highly-opinionated checks that might really flag code that isn't wrong but violates a project's coding conventions. Static-linters can also raise false-positives
    • in any event, this became an issue for using google-benchmark. I wrote a workaround to deal with this
      • the workaround is verbose and ugly, but there isn't a great alternative given the minimum required cmake version
      • I made it very clear how we can drastically simplify the logic in a few years, when we start requiring CMake >= 4.2
  3. I wrote wrappers for CMake's FetchContent_Declare and FetchContent_MakeAvailable
    • Background context:
      • CMake provides the find_package command to let builds search for preinstalled dependencies and FetchContent to download source-code for a dependency and build the dependency as an embedded part of the GrackleBuild.
      • While usage of FetchContent makes Grackle Builds easier for people less-experienced with CMake, the recommended best-practice is to make it possible for project consumers to actually provide a precompiled version of a dependency, if they want.
      • We were indeed doing this up until now. However the logic was little hacky that limited control because CMake historically didn't have a standard way to handle this.
      • However, CMake 3.24 did a bunch of work to unify the usage of find_package and FetchContent. A large part of this involves passing the FIND_PACKAGE_ARGS keyword argument to FetchContent_Declare
    • The wrappers in this PR effectively make it possible to pass the FIND_PACKAGE_ARGS keyword argument to FetchContent_Declare
      • when using a CMake version < 3.24, the wrappers emulate support for the FIND_PACKAGE_ARGS keyword.
      • otherwise, the wrappers just directly forward arguments onto CMake's FetchContent_Declare and FetchContent_MakeAvailable
    • This is useful for a few reasons:
      1. As we add more dependencies, the hacky solution we were using would have gotten increasingly messy (because it would involve a bunch of if-else statements, especially if we tried to consolidate calls to FetchContent_MakeAvailable). Instead, we now use well-established logic.
      2. Advanced CMake users and Simulation codes built with CMake that consume Grackle can now reap the benefits of passing FIND_PACKAGE_ARGS to the FetchContent_Declare when they use CMake versions >= 3.24.
      3. It will now be trivial to update the logic when we bump the minimum required CMake version to 3.24. This is somewhat useful for ensuring that logic pertaining to disabling static-linters also won't change much when that time comes.

@mabruzzo mabruzzo added the build-system Related to the build-system label Feb 9, 2026
@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Feb 10, 2026

I think we need to bump the gold standard cache. I didn't make any changes in this PR that would directly lead to this.

All I did was bump the docker image. This makes some sense given experience on my own machine (switching between Python versions often requires me to reset the gold-standard)

EDIT: I did in fact bump the gold standard cache and things worked out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build-system Related to the build-system

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant