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

Set link_directories for exported Geant4 target #3086

Merged
merged 1 commit into from
Mar 13, 2020

Conversation

qgp
Copy link
Collaborator

@qgp qgp commented Mar 4, 2020

No description provided.

@@ -22,6 +22,13 @@ set_target_properties(geant4
PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
"${Geant4_INCLUDE_DIRS}")

# TODO: fix upstream and provide proper variable for library directories
# for now derive from Geant 4 include dir (first in list)
list(GET Geant4_INCLUDE_DIRS 0 Geant4_INCLUDE_DIR)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my education, is this fixing some problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sawenzel Yes, it ensures consistent usage of the settings as detected (and cached) by cmake. In other words, the build is then fully configured by cmake and does not require the Geant4 environment to compile. Together with alisw/alidist#2121 it also fixes recompilation from the BUILD directory without the aliBuild environment.

Do you see an issue with this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. just wanted to understand the rational.

@aphecetche
Copy link
Collaborator

@gqp hum, this looks fragile to me... The very name lib for lib dir is assuming we're not targeting platforms/distributions which are using variants like lib64, etc.. Also, we're betting the relative path from include to lib won't change in Geant4...

So, as rightfully stated in your own comment in the file it should ideally be handled upstream. @ihrivnac do you know if that's something that is considered already ?

@qgp
Copy link
Collaborator Author

qgp commented Mar 5, 2020

@aphecetche Thanks. Indeed, this is an ugly work-around for the fact that Geant 4 does not expose its library path. However, not setting this in FindGeant4 and relying on the linker to find a library somewhere (which is not even guaranteed to be the one corresponding to the include dir used) is simply broken. So, until fixed upstream, we will have to do some heuristics here. I will try and make it more robust with a call to find_library() on a suitable base directory.

@ihrivnac
Copy link
Contributor

ihrivnac commented Mar 5, 2020

Hi,
There is indeed a lot of progress in the last Geant4 version (10.6.p01); see:
http://geant4-data.web.cern.ch/geant4-data/ReleaseNotes/ReleaseNotes4.10.6.html
=> 9. Detailed list of changes and fixes, CMake

We are however still using 10.4.p2, and my PR for 10.5.p01 since last Friday (alisw/alidist#2114) is failing due to a problem on the test machine.

@qgp
Copy link
Collaborator Author

qgp commented Mar 5, 2020

@ihrivnac Ok, 10.6 indeed seems to address many of the cmake shortcomings. Is it planned to move to 10.6 anytime soon? If this is not imminent, I would still try to fix FindGeant4 for now. With the move to 10.6 FindGeant4 (and probably other pieces) would then have to be touched anyway, also to properly use the exported targets instead of the current usage of Geant4_INCLUDE_DIRS.

@ihrivnac
Copy link
Contributor

ihrivnac commented Mar 5, 2020

If there are no objections from the simulation WP, we can move to 10.6; as by moving to 10.5 we will anyway go for a different version than the one used in production.
I am adding @sawenzel to the thread.

@sawenzel
Copy link
Collaborator

sawenzel commented Mar 6, 2020

I think we already discussed this and from my side we can go to 10.6.

@qgp qgp force-pushed the geant4_cmake branch 2 times, most recently from f21e8fe to f3c0856 Compare March 10, 2020 15:54
@qgp
Copy link
Collaborator Author

qgp commented Mar 10, 2020

@aphecetche This should be more robust now and provide the required information (before the move to 10.6 happens). Let me know if you have further suggestions.

@qgp qgp marked this pull request as ready for review March 10, 2020 15:55
@qgp qgp requested a review from a team as a code owner March 10, 2020 15:55
@qgp qgp changed the title Set link_directories for exportet Geant4 target Set link_directories for exported Geant4 target Mar 11, 2020
@ktf ktf requested review from sawenzel and aphecetche March 12, 2020 20:20
Copy link
Collaborator

@aphecetche aphecetche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. And to be revisited when we move to Geant4 10.6. @qgp @ihrivnac will you e.g. create a JIRA ticket so it's not forgotten ?

@sawenzel sawenzel merged commit 76ba4d4 into AliceO2Group:dev Mar 13, 2020
@qgp
Copy link
Collaborator Author

qgp commented Mar 13, 2020

@aphecetche Yes, to be revisited once we move to 10.6, I am available for this, of course.

@ihrivnac Can you take care of the JIRA ticket or whatever it needs to get going with the upgrade to 10.6? You know the situation there much better than I do. Keep me in the loop, please.

@ihrivnac
Copy link
Contributor

Ok, I will take care of the JIRA ticket.

@qgp qgp deleted the geant4_cmake branch March 13, 2020 10:22
@ihrivnac
Copy link
Contributor

Issue is created https://alice.its.cern.ch/jira/browse/O2-1242

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants