cmake: don't gate FetchContent_Populate(rocm-cmake) on the source dir already being absent - #5
Open
pvelesko wants to merge 1 commit into
Open
cmake: don't gate FetchContent_Populate(rocm-cmake) on the source dir already being absent#5pvelesko wants to merge 1 commit into
pvelesko wants to merge 1 commit into
Conversation
… already being absent
rocm-cmake_SOURCE_DIR is a plain CMake variable, set only when
FetchContent_Populate actually runs. It does not persist between separate
`cmake` invocations the way a cache entry does. The `if(NOT EXISTS
"${FETCHCONTENT_BASE_DIR}/rocm-cmake-src")` guard added in d340aa2 wrapped
the whole Declare/GetProperties/Populate block, so on any configure of a
build tree where rocm-cmake had already been fetched by an earlier
invocation, that guard evaluated false, Populate never ran, and
rocm-cmake_SOURCE_DIR came back empty -- causing find_package(ROCM ...
HINTS "${rocm-cmake_SOURCE_DIR}") to fail with "ROCMConfig.cmake ... The
file was not found", even though it was sitting on disk right where the
earlier fetch had put it.
This was invisible as long as every build started from a wiped build
directory, but hits immediately once anything reconfigures an existing
tree -- in practice, a build interrupted (job cancelled, runner restarted)
after the fetch but before find_package ever ran and cached ROCM_DIR:
the next configure of that same tree has _deps/rocm-cmake-src on disk and
no cached ROCM_DIR to short-circuit past the bug.
FetchContent_Populate is already idempotent across configures on its own
(it consults a stamp file under the subbuild dir and skips the network
fetch when it is already there), which is the whole reason it is safe to
call unconditionally every configure; the outer guard here was redundant
with that and broke the one thing Populate is relied on to still do even
on a no-op call: setting rocm-cmake_SOURCE_DIR for the current process.
Keeping FetchContent_Populate (not switching to FetchContent_MakeAvailable)
per d340aa2, which chose it specifically to keep rocm-cmake's own test
suite out of this project's CTest.
Verified in isolation: a minimal FetchContent+find_package pair using this
exact structure fails on a second configure of a tree whose first
configure only ran the FetchContent half (simulating an interrupted first
run) with the old guard, and succeeds with it removed.
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.
rocm-cmake_SOURCE_DIR is a plain variable, only set when FetchContent_Populate actually runs; it does not persist between separate cmake invocations. The
if(NOT EXISTS ...)guard added in d340aa2 wraps the whole Declare/GetProperties/Populate block, so reconfiguring a build tree where rocm-cmake was already fetched skips Populate, leaves the variable empty, and find_package(ROCM ... HINTS "${rocm-cmake_SOURCE_DIR}") fails with "ROCMConfig.cmake ... not found" even though the file is on disk.Hit this in chipStar CI (PR #1442, build-and-test-libraries) once a build dir got reconfigured after an interrupted prior run left _deps/rocm-cmake-src populated but never reached find_package to cache ROCM_DIR.
FetchContent_Populate is already idempotent across configures (stamp-file gated), which is why it's safe to call unconditionally; removing the redundant outer guard restores that. Keeps FetchContent_Populate rather than FetchContent_MakeAvailable, matching d340aa2's reason for that choice (keeps rocm-cmake's own tests out of this project's CTest).
Verified in isolation with a minimal FetchContent+find_package repro using the same structure: fails on a second configure with the old guard when the first configure only got as far as populating (simulating an interrupted run), succeeds with it removed.