Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix weird edge cases when trying to find_package(Celeritas) #1586

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Jan 17, 2025

#1573 saw some weird failures against Geant4 11.1 and 11.2 but not 11.0 nor 11.3. The failures occurred in the second invocation of find_dependency(XercesC) (missing XercesC_INCLUDE_DIR) but went away if we added a REQUIRED argument to find_package(Celeritas).

Ultimately there were several factors at play:

  1. find_dependency caching suppressed the failing XercesC only when REQUIRED was set
  2. VecGeom and Celeritas both set XercesC_INCLUDE_DIR as non-cache variables, and Geant4 set it as a cache variable
  3. Old CMake policy CMP0102 causes the mark_as_advanced in FindXercesC.cmake to write an empty cache variable if a normal variable is set
  4. Old CMake policy CMP0126 then causes the find_path command to overwrite the normal variable with the empty cache value (on the next call to FindXercesC)
  5. Maybe policy CMP0011 which protects policy scopes wasn't working??
  6. The real underlying cause: Geant4 11.1 included PTL, which had a cmake_minimum_required(3.8) that reset all the policies to that ancient value. This was fixed in Use CMake version range to support CMake 3.30 and newer jrmadsen/PTL#51 and the fix was merged into Geant4 11.3.

I've bypassed the root cause of normal/cache mismatch by propagating the CACHE status to the CeleritasConfig.cmake macro. I also added policy scoping to try to limit the damage the other scripts can do.

This was a confluence of a policy overwrite from PTL, CMake policy
CMP0102 (where `mark_as_advanced` writes an empty cache variable if a
local variable is named), and `find_path` which has some funny logic for
pulling from cache/local values (and maybe CMP0126 as well).
@sethrj sethrj requested review from drbenmorgan and pcanal January 17, 2025 19:57
Copy link

Test summary

 4 219 files   6 521 suites   15m 4s ⏱️
 1 677 tests  1 671 ✅  6 💤 0 ❌
22 976 runs  22 896 ✅ 80 💤 0 ❌

Results for commit 56a3fa1.

@sethrj sethrj requested a review from stognini January 18, 2025 01:36
@sethrj sethrj mentioned this pull request Jan 18, 2025
10 tasks
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.

1 participant