-
Notifications
You must be signed in to change notification settings - Fork 513
gasnet: add more variants to spack package #1409
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
base: develop
Are you sure you want to change the base?
Conversation
bonachea
left a comment
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.
This PR greatly expands the variant space of the GASNet spackage.
As such, I'm very interested in thoroughly testing and reviewing this PR.
Unfortunately I'm under water this week and next with the INCITS/J3 Fortran standards meeting, so realistically I might not get to until the first week of November.
In lieu of a "real" review, here are some requests I'd have based on a quick skim:
- GASNet's documented configure defaults were chosen carefully over time and experience to balance the needs of multiple clients and target systems. In order to satisfy the principle of least surprise for Spack users and package authors, any Spack variants corresponding to a GASNet configure option should have matching default behavior (unless there's a very compelling reason to diverge). Offhand the following variants currently appear to fail this criteria:
pic,par,pthreads. - Some GASNet configure options default to "smart" detection of available dependencies, which improves the install experience. Corresponding Spack variants should default to using that smart detection unless the client asked for a specific variant (again, unless there's a very compelling reason to diverge). IOW the Spack default for these should be
auto(or equivalent) with no corresponding configure argument. Offhand the following variants currently appear to fail this criteria:pmi,pthreads. - The dependency on
cray-pmipackage is pointless and degrades the casual user experience. That package doesn't actually install anything (so it's just forcing manual addition of an external), and GASNet configure is already capable of finding the system Cray PMI installation without Spack's help. - The
segmentvariant doesn't appear to be "wired-up" at all?
@PHHargrove may have more available time to help audit places where the Spack variant default behavior doesn't match the GASNet configure defaults.
|
@bonachea thanks for the initial comments. I'll look at these points in the coming days. I'm expecting we can accommodate some of the automatic detection that should stay. Others will require some duplication in logic within the Spack package. The reason for this is a difference in responsibilities between Spack and the configure script. Some decisions in the Spack concretization we will not be able to postpone until the configure script runs. As carefully crafted that configure script is, some of the problems it solves are done by Spack and its recipes before the script even runs. Therefore its capabilities become less relevant. |
9014b9f to
73b7463
Compare
73b7463 to
86923fc
Compare
|
@bonachea @PHHargrove here is a new iteration based on your initial feedback. 1 & 2. Many variants now default to I've also added external detection. |
|
@rbberger Thanks for the updates! This is a very busy week for us, so realistically I won't have time for an in-depth review until next week. |
Changes moved out of #1231. @bonachea @PHHargrove please review