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

Provide option to select between G4VG via FetchContent or find_package #348

Merged
merged 4 commits into from
Feb 18, 2025

Conversation

drbenmorgan
Copy link
Member

A small ease of use/integration enhancement to use an external install of G4VG by default, falling back to use of FetchContent otherwise.

This should allow easier integration in ATLAS/Athena as well as common integration with Celeritas so the same G4VG install is used without symbol clashes.

@phsft-bot
Copy link

Can one of the admins verify this patch?

@JuanGonzalezCaminero
Copy link
Contributor

Thanks Ben, works well for me.
Could you add a status message to make it clear whether the local install or remote version has been used?

@drbenmorgan
Copy link
Member Author

I've added the message, but note that @SeverinDiederichs has also reported some issues at the configure step we're trying to work out.

@drbenmorgan
Copy link
Member Author

We worked out the issue the @SeverinDiederichs was having. It's not a blocker as one can still build and install, but the warnings can be confusing. The sequence of operations and problems is:

  1. Build and install AdePT in /some/path without a preexisting install of G4VG available

  2. This will install install the internally built G4VG alongside AdePT

  3. Subsequent CMake runs can then pick up that G4VG install via the find_package call, given warnings of the form:

    CMake Warning at test/CMakeLists.txt:10 (add_executable):
      Cannot generate a safe runtime search path for target test_atomic because
      there is a cycle in the constraint graph:
    
        dir 0 is [/adept-build/BuildProducts/lib64]
          dir 1 must precede it due to runtime library [libg4vg.so]
        dir 1 is [/some/path/lib64]
          dir 0 must precede it due to runtime library [libAdePT_G4_integration.so]
    
      Some of these libraries may not be found correctly.
    Call Stack (most recent call first):
      test/CMakeLists.txt:72 (build_tests)
    

This requires a little more debugging first to double check the interplay between CMAKE_INSTALL_PREFIX, CMAKE_PREFIX_PATH and LD_LIBRARY_PATH. One solution is probably to have an option to only use the builtin G4VG when requested.

@drbenmorgan
Copy link
Member Author

Note that the above warnings still allow a successful build/install, but ideally we want to be rid of them to avoid confusion or possibly incorrect build setups from being created.

Simple "find_package then FetchContent" logic (whether directly or
through the coupling capability) can lead to CMake-generate time errors
if CMake is run where:

1. The previous CMake invocation set things up to use FetchContent
2. The current CMake invocation picks up an external G4VG through
   find_package.

CMake-generate will then warn about runtime paths, e.g.:

"""
 Cannot generate a safe runtime search path for target test_atomic because
  there is a cycle in the constraint graph:
"""

This does not stop generation or a subsequent build from completing, but
is confusing. This can be considered a temporary problem until AdePT
moves entirely to required an external G4VG only (As was done with
integrating G4HepEm).

Provide a temporary workaround through a CMake option to control whether
to get G4VG via FetchContent or find_package. Default to use
FetchContent matching current behaviour (ON). This will also not trigger
the issues noted above under any case. Switching the option from ON
to OFF in a given build directory can trigger the above warnings, but
they can no longer happen without the developer making that decision.
@drbenmorgan drbenmorgan changed the title Find external G4VG or fallback to FetchContent Provide option to select between G4VG via FetchContent or find_package Feb 13, 2025
@drbenmorgan
Copy link
Member Author

O.k., the latest commit does two main things

  1. Adds the requested message to report whether G4VG is being provided by FetchContent or find_package.
  2. Uses a CMake option to choose between these
    • It defaults to use FetchContent as this matches what we currently do, and is probably best whilst the remaining issues are ironed out as it makes it trivial to pull in a given commit upstream if required.
    • If set to OFF it'll use find_package instead.

This makes things a bit more robust/less subject to confusion. It doesn't totally fix @SeverinDiederichs's issue, but prevents it happening without a deliberate change in the build options. Whilst there may be ways to make this fully robust, I think the true long term solution will be to move completely to only requiring an external install of G4VG. But, this is a good enough solution for now to avoid confusion.

@JuanGonzalezCaminero JuanGonzalezCaminero merged commit 1584b6f into apt-sim:master Feb 18, 2025
3 checks passed
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.

3 participants