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

Check if the CLHEP target exists before creating it #1169

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

jmcarcell
Copy link
Member

@jmcarcell jmcarcell commented Sep 15, 2023

BEGINRELEASENOTES

  • CMake: DDG4: Check if the CLHEP target exists before creating it in DD4hepBuild.cmake, since if it exists cmake will error.

ENDRELEASENOTES

@andresailer
Copy link
Member

This happens with which version of Geant4?

@jmcarcell
Copy link
Member Author

This happens with a Geant4 installed with the builtin CLHEP and a separate installation of CLHEP when trying to build several packages that depend on DD4hep at the same time, either the CLHEP::CLHEP target is being set several times by the file changed in this PR or it is being set from somewhere else; in any case this fixes it.

@andresailer
Copy link
Member

Uhh, that looks like a recipe for disaster and potential header / library mismatch.

@jmcarcell
Copy link
Member Author

But I think it's irrelevant for this PR having two installations, the same error would happen with a single installation having the builtin CLHEP from Geant4 and building several packages at the same time (this doesn't happen with a single package depending on DD4hep but it happens with two)

@andresailer
Copy link
Member

If we have a Geant4 builtin CLHEP, we should still be using the Geant4 builtin CLHEP here though, so we shouldn't use the already existing CLHEP::CLHEP target.

@github-actions
Copy link

Test Results

       6 files         6 suites   6h 46m 54s ⏱️
   355 tests    355 ✔️ 0 💤 0
1 055 runs  1 055 ✔️ 0 💤 0

Results for commit 8e9f5d1.

@jmcarcell
Copy link
Member Author

How do we know the CLHEP::CLHEP target isn't coming from the builtin version in G4? I checked and it's Gaudi who is setting CLHEP::CLHEP so find_package(DD4hep) after find_package(Gaudi) doesn't work. I think this should still not fail and be a warning instead, as it failing doesn't guarantee things are going to fail (something else using the builtin CLHEP from G4 and creating a CLHEP::CLHEP).

@andresailer
Copy link
Member

At least until now, Geant4 didn't create a CLHEP::CLHEP, which is why we are creating it here. So if there is CLHEP::CLHEP, it didn't come from Geant4. Which is why I initially asked which version of Geant4 this happens with.

@jmcarcell
Copy link
Member Author

Yep it's Gaudi as I said, so you could have a Geant 4 with CLHEP builtin and Gaudi using that CLHEP and creating CLHEP::CLHEP but then find_package(DD4hep) doesn't work, it's definitely a weird case and something that won't happen for most people nor in the stacks.

@andresailer
Copy link
Member

Yes, it is a weird case, which should be avoided, i.e., use external CLHEP for Geant4.

At least with the warning we get some indication that this might be causing issues. (Probably only if you run Geant4 inside Gaudi, which I personally don't do much...)

@andresailer
Copy link
Member

Could you rebase and squash please?

@jmcarcell
Copy link
Member Author

jmcarcell commented Sep 15, 2023

Could you rebase and squash please?

Done

Yes, it is a weird case, which should be avoided, i.e., use external CLHEP for Geant4.

I agree. It just happens that my Linux distributions ships geant4 or the geant4 recipes with the builtin CLHEP which is still useful to check that things compile, running there can be a mismatch of CLHEP versions...

@andresailer andresailer enabled auto-merge (rebase) September 15, 2023 14:39
@andresailer andresailer merged commit df64551 into AIDASoft:master Sep 15, 2023
13 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.

2 participants