-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add an ACTS package #10600
Add an ACTS package #10600
Conversation
b5c26de
to
ae61261
Compare
17ff1ed
to
bed35a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @HadrienG2
Nice PR, I had already an ad-hoc recipe for the FCC software on a different so I put some questions below.
Also, in my recipe I had only support for versions 0.5.3
and 0.07.01
, which I think add a couple of if-else
statements to the recipe since there were significant changes between those versions and 0.8:
, however those can be considered on a later PR (the current recipe we use in FCC is here)
|
||
depends_on('cmake @3.7:', type='build') | ||
depends_on('boost @1.62: +program_options +test') | ||
depends_on('eigen @3.2.9:', type='build') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you seen problems finding eigen
? I had to add this to the cmake_args
:
args.extend(["-DEIGEN_INCLUDE_DIR=%s" % spec['eigen'].prefix + "/include/eigen3"])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake's Eigen include path detection is... finnicky to say the least. I needed similar things for my manual builds, but strangely enough things Just Work out of the box when building using spack. I suspect that Spack must set some CPATH environment somewhere to make this work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HadrienG2 FYI, Spack doesn't set CPATH but injects include paths into the command line. See e.g. #10623 and the other PRs referred therein.
Regarding choice of versions, I usually tend to only include the latest release(s) of a package in my PRs for new spack packages, because older releases tend to need more build hacks and most HEP experiments tend to stay very close to the current package versions, making old releases rarely needed. Now, if you tell me that you are still using some of the older releases and have a good reason to stick with them, I don't mind adding support for them of course 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to move on to 0.08.01
so I think it is fine as it is.
This is a build recipe for the ACTS HEP tracking toolkit.