-
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
spec : interface for build information #1875
spec : interface for build information #1875
Conversation
@tgamblin @davydden @adamjstewart @scheibelp @becker33 If you have time I would be interested in knowing what you think about this PR |
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.
Regarding the opt-out
, probably depends on how many packages actually can implement this. Probably keeping it on
by default is ok.
One more thing you may consider is extra link flags. blas/lapack
compiled with openmp
need to add extra flags self.compiler.openmp_flag
. So everyone who use blas/lapack
should be able to use those, see https://github.com/LLNL/spack/blob/develop/var/spack/repos/builtin/packages/openblas/package.py#L128-L129 . I don't think you want to mix them with cppflags
.
Also we need to agree on naming, cppflags
return a string, whereas libs
return an array/list or alike. This is not clear from names. One way would be to return a string for singulars (cppflag
or ld_flag
) and arrays/lists for plurals (libs
, dirs
).
shared = True if '+shared' in self.spec else False | ||
return find_libraries( | ||
['libopenblas'], root=self.prefix, shared=shared, recurse=True | ||
) | ||
|
||
@property | ||
def lapack_libs(self): |
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.
your current design appears to be limited in case of a single provider supplying several things, like mkl
does for blas
, lapack
and scalapack
. In this case, the libs for the last one (scalapack
) are certainly different from those used in blas
and lapack
. In other words
spec['blas'].libs
, spec['lapack'].libs
and spec['scalapack'].libs
could all be provided by the same package and potentially could all be different. Note that it's different from what you do with a query parameter.
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.
@davydden Limited in which sense ? Suppose mkl
is used, then if you code :
l = spec['scalapack'].libs
this will in order try to grab / call:
Mkl.scalapack_libs
Mkl.libs
_libs_default_handler
If you need specialization you can code :
class Mkl(Package):
@property
def scalapack_libs(self):
...
@property
def lapack_libs(self):
...
and take appropriate measures based on what you were asked.
There's a little caveat in that I didn't take care so far to return copied specs on __getitem__
call, so that things like :
a = spec['blas']
b = spec['lapack']
...
don't interfere with each other if they are provided by the same package. That is a detail I'll work on later if this syntax / machinery looks good enough...
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.
@davydden To add more on that : openblas
is fairly well-behaved, so with this implementation you can even remove Openblas.libs
and have the code still working. The default handler will do just the right thing...
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.
oh, i see, thanks for clarifying. Then this
Mkl.scalapack_libs
Mkl.libs
_libs_default_handler
should be perfectly fine indeed.
This is definitely one of the thing we should discuss carefully iff the general idea is good enough. I already don't agree with singulars vs. plurals but let's save this fight for later in case 😄 |
lib/spack/spack/spec.py
Outdated
@@ -537,9 +538,120 @@ def __str__(self): | |||
["^" + self[name].format() for name in sorted(self.keys())]) | |||
|
|||
|
|||
def _libs_default_handler(descriptor, spec, cls): | |||
"""Default handler when looking for 'libs' attribute |
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.
i see now what you mean about openblas
being simple. 👍
For partial opt-out one could do
This would prevent anyone from using |
@@ -46,24 +46,25 @@ class SuperluDist(Package): | |||
depends_on('metis@5:') | |||
|
|||
def install(self, spec, prefix): | |||
lapack_blas = spec['lapack'].lapack_libs + spec['blas'].blas_libs | |||
lapack_blas = spec['lapack'].libs + spec['blas'].libs |
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.
neat!
class MyPackage(Package):
libs = None
... will have the same effect : retrieving |
c7b5574
to
3ef07a6
Compare
3ef07a6
to
2e7f958
Compare
Came across another reason why this PR is good to have: on |
@tgamblin I've noticed this entered the project v0.10 in the |
8c47aee
to
09f7405
Compare
@tgamblin @adamjstewart @becker33 @scheibelp I think this is ready for review. I edited the description above to have all the information I can remember right now. I think the method you would like to pay the most attention when reviewing is |
'@BLAS_LIBS': self.spec['lapack'].blas_libs.joined(), | ||
'@LAPACK_LIBS': self.spec['lapack'].libs.joined(), | ||
'@BLAS_LIBS': self.spec['blas'].libs.joined(), | ||
# FIXME : what to do with compiler provided libraries ? | ||
'@STDCXX_LIB': ' '.join(self.compiler.stdcxx_libs) |
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.
What to do with compiler provided libraries?
Not sure, but it definitely needs some work. For example, we just added self.compiler.pic_flag
because compilers like NAG use -PIC
instead of -fPIC
. The problem is that if I mix compilers (GCC + NAG), the flag isn't right for each compiler.
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.
I agree and the FIXME
is there because I think compiler flags deserve a PR on their own. Especially if the effort to turn them into some sort of dependencies already started.
msg += ' At most one is admitted.' | ||
raise KeyError(msg) | ||
|
||
name, query_parameters = query_parameters[0], query_parameters[1:] |
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.
It looks like query_parameters[1:]
could just be query_parameters[1]
And actually since this must be of length two, you can just do:
name, query_parameters = query_parameters
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.
@scheibelp The most frequent case is the one with zero query parameters:
>>> l = [1]
>>> a, b = l[0], l[1:]
>>> print(a, b)
(1, [])
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.
Got it - thanks!
At a high level, I'm curious why this approach is preferable to e.g. keeping the getattr syntax and then for example changing Openmpi.libs to be a function that accepts arguments like 'cxx'. I suppose that forces users to say something like I'm curious because in my opinion this approach adds complexity relative to sticking with an override of getattr. For example stuff like:
makes me cautious. Perhaps I'm overlooking other major benefits that the getattr approach lacks. |
3b5282c
to
605aeb4
Compare
ping |
@alalazo: I think there is still some refactoring that can be done here w.r.t. consolidating the handlers, I want to get the feature in, though. Can you add docs for users in another PR? The docs should probably just talk about the conventions (add a method to your package so you can get at it via |
@tgamblin Will do. Do you have any preference where to put the docs? |
@alalazo: probably should be a section in the packaging guide; maybe it could be explained along with |
- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily
- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily
- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily
- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily
- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily
TLDR
This PR provides a general mechanism to forward queries from
Spec
toPackages
using the descriptorForwardQueryToPackage
. It implements most of the ideas discussed in #1821 (see details below), including a default behavior for the methodsspec.libs
andspec.cppflags
.To showcase an example some of the packages in the dag:
have been reworked to make use of the new features.
Analysis and requirements
There are a number of things that emerged from #1682 and #1821 concerning build information, that I'll try to recap below for ease of reference :
lapack_shared_libs
and similar properties #1682 is needlessly redundant (e.g.spec['blas'].blas_libs
mentionsblas
twice)blas
andlapack
providers are taken care of properly, while normal packages are not__getattr__
to intercept calls to non-existing attributes inSpec
should be avoideddealii
andtrilinos
)On top of that I would add a couple more points :
openmpi
installscxx
,c
andfortran
APIs and a particular package may need to link withcxx
)spec['name'].libs
will require very often to search forlib{name}.{suffix}
somewhere inprefix
)Design
I grabbed the basic idea that @tgamblin proposed in #1821 and expanded it further. What I didn't like there was just the syntax it would have imposed :
so I tried to achieve something like :
And have readable tracebacks in case of failures :
The whole design is based on a few of simple concepts :
Spec
instance has now an internal state to keep track of 'queries' from client code, and a few attributes to retrieve, set or clear the query stateSpec
toPackage
and uses a chain of responsibility (documented in the code) to serve themSpec.__getitem__
automatically set a new query state in the requiredspec
. If thespec
being queried is virtual__getitem__
returns a copy instead of a reference to deal with the case of packages that provide more than one service.Modifications
__getitem__
cppflags
andlibs