Skip to content

CMakeLists: use list(APPEND) for CMAKE_MODULE_PATH#550

Merged
markcmiller86 merged 1 commit into
llnl:mainfrom
sbryngelson:fix-cmake-module-path-append
May 11, 2026
Merged

CMakeLists: use list(APPEND) for CMAKE_MODULE_PATH#550
markcmiller86 merged 1 commit into
llnl:mainfrom
sbryngelson:fix-cmake-module-path-append

Conversation

@sbryngelson

Copy link
Copy Markdown
Contributor

Problem

CMakeLists.txt currently uses set() to register Silo's CMake module directory:

set(CMAKE_MODULE_PATH ${Silo_SOURCE_DIR}/CMake)

set() replaces CMAKE_MODULE_PATH entirely, silently discarding any value passed by a parent or superbuild system via -DCMAKE_MODULE_PATH=.... This means any custom FindXXX.cmake modules injected by the parent are lost before SiloFindHDF5.cmake runs.

Impact

This breaks superbuild systems that embed Silo as a dependency and need to inject custom CMake finders for non-standard environments. A concrete example: MFC, an exascale CFD solver, targets Cray HPC clusters where CMake's built-in FindHDF5.cmake does not locate HDF5 correctly. MFC injects a Cray-specific finder via -DCMAKE_MODULE_PATH=.... With the current set(), that path is wiped out, and the build fails:

CMake Error at CMake/SiloFindHDF5.cmake:92 (message):
  An explicit request for HDF5 was made but HDF5 was not found.
  You may want to try setting SILO_HDF5_DIR

MFC currently works around this by patching Silo at build time, but that patch exists solely because of this one line.

Fix

Change set() to list(APPEND):

list(APPEND CMAKE_MODULE_PATH "${Silo_SOURCE_DIR}/CMake")

This preserves any externally-set module paths while still making Silo's own CMake/ directory available — standard CMake practice for project-internal module directories.

No behavior change for standalone Silo builds where CMAKE_MODULE_PATH is empty or unset at entry.

set() silently discards any -DCMAKE_MODULE_PATH=... passed by a
parent or superbuild system. list(APPEND) preserves those paths
while still registering Silo's own CMake/ directory, which is
standard CMake practice for project-internal module directories.
@markcmiller86

Copy link
Copy Markdown
Member

Thanks for this PR 💪. Will merge tomorrow and backport to 4.12RC branch after that.

Just curious, was anything about this PR AI-assisted?

@markcmiller86
markcmiller86 merged commit ec8f24e into llnl:main May 11, 2026
6 checks passed
@sbryngelson

Copy link
Copy Markdown
Contributor Author

The entire thing, minus a final pass by me.

@markcmiller86

Copy link
Copy Markdown
Member

The entire thing, minus a final pass by me.

Ok, thanks for following up with that.

I can't put my finger on it but the PR commit comment read like it was.

FWIW, because the use of AI is very much a topic of substantial discussion especially in open source code development, I myself am pretty sheepish about submitting any work or information to others that is derived even in part from AI assistance without at least attributing it as such...if for no other reason that I might inadvertently giving the others the impression I am more skilled than I a actually am 😉.

@sbryngelson

Copy link
Copy Markdown
Contributor Author

@markcmiller86 I'm sympathetic to that view. I worry less about the skilled part than just making silly mistakes the reviewer won't notice. Usually, I only do this on my own code, so I don't have to dwell on the idea much.

In cases where I've made PRs to others' code, they've all been one- or two-line changes or backed by their existing tests, so I worry a bit less. If the maintainer can't review a one-line change, something else is surely amiss!

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