there are two functions in the utilities to_lib_name and to_link_flags, which you could substitute with functionality of the new class.

Looks good!

@davydden Thanks for the thorough review! I'll start working on the PR + cleaning and documenting the code.I'll come back to you when all this will be ready for a test-drive 😄

@tgamblin @adamjstewart ready to be reviewed

@alalazo is it rebased on top of develop? I can test it on macOS now.

@davydden Yes, it should be

Would there be any incentive to add support for include flags in addition to library linker flags?

@adamjstewart I would do it in another PR, as it requires some thinking on the searching logic

@KineticTheory: if this does not clobber your changes from #1539, I think it's ready to merge. Can you take a look?

This PR looks pretty good. There's only a couple of things I think it's missing. I would like find_libraries to accept either a list or a string, since most of the time we will be looking for a single library. Right now it only accepts a list or other sequence. I also think we could use quite a bit more documentation on what methods are available and what they return.

@adamjstewart let's get it merged and then fix minor issues in follow up PRs.

NOTE: this is not a request for changes. This is a proposal for how this PR should evolve because I think it's heading in the right direction and it solves a problem that other packages have.

One thing I thought about when looking at this PR: I don't really like it that some dependents still search for their dependencies libraries, though I realize that it can be necessary. Basically there are two guiding principles here:

1. The dependencies know themselves best, and if they can expose their build information consistently, they should do that and let dependents access it.
2. Dependents should prefer to ask dependencies for build information (lib locations, etc.) and only search if necessary. Searching complicates build logic and Spack should already know where things are because it already installed the dependencies.

(2) breaks down for external packages, which Spack didn't install and which may even be laid out differently than Spack would lay them out (e.g. a install of boost might have al the weird lib suffixes that boost supports, and I think Spack wouldn't currently know what to do with that). Even better is that find_libraries is hidden behind a property interface so the client code doesn't know how the library names are being assigned.

This PR follows both of these principles for LAPACK and BLAS. Using properties on specs for things like blas_libs and lapack_libs is great. It doesn't follow these for fftw, arpack, superlu, etc. See the install method for armadillo, which does its own find routines.

I think this PR makes it clearer to me what a good convention for passing args among packages in a build should look like. You want to ask the package for a property, and the package needs to know a) the property name and b) the vdep it's being asked as. For non-virtual packages, (b) doesn't matter and is always just the plain package name, but when you're asked to provide the libs/headers for a particular interface, you need to know the interface name.

I guess the question is whether this should be a convention or a more formal interface. The convention currently looks like:

    spec['blas'].blas_libs
    spec['lapack'].lapack_libs

Slightly annoying things about it:

1. You have to write the interface twice even though you know what you asked for.
2. The interface name is written once as a string and once as a property name prefix.

So, think we could have a convention like this:

def provider_property(function):
    """Decorator for provider properties that get info from dependencies."""
    def __init__(self, name):
        self.name = name

    def __call__(self, provider):
        """Get provider-specific property if available, or default to generic property."""
        specific = provider.replace('-', '_') + '_' + self.name
        dep = spec[provider]
        if hasattr(dep, specific):
            return getattr(dep, specific)
        else:
            return getattr(dep, self.name)  # default to generic attribute


class Spec(object):
    ...
    libs = provider_property('libs')
    includes = provider_property('includes')

Now the interface looks like this:

    spec.libs('blas')
    spec.libs('lapack')

And you've got a convention for properties to add to packages (libs, <provider>_libs, etc.). Package could provide a default implementation:

class Package(object):
    ...
    def libs(self):
        # requiring installation allows us to be more generic b/c we can search,
        # but it does restrict when you can call the method.
        assert(self.installed)    

        return find_libraries(... something that returns all the libs in `lib` or `lib64`...)

And packages that need to can add things like blas_libs, lapack_libs, superlu_dist_libs, etc. as they do in this PR. Packages like arpack and superlu can do nothing b/c we find their libs automatically and they're available through, e.g. spec.libs('arpack').

I guess the question is whether the interface above is so much better than the simple convention we've got that it's worthwhile. The current convention is not so bad. I think it boils down which is simpler to document and teach to package authors.

The other question is whether this convention is worthwhile for non-virtuals. Sometimes the client needs to be specific about which lib names it needs, in which case find_libraries seems like the right thing to do.

What do you all think?

1. You have to write the interface twice even though you know what you asked for.

one have to do that because a single package can provide different virtual packages (e.g. mkl provides blas and lapack and scalapack). Would the convention you suggested work here as well?

The other question is whether this convention is worthwhile for non-virtuals. Sometimes the client needs to be specific about which lib names it needs, in which case find_libraries seems like the right thing to do.

i would say for most packages the library names are simple (libarpack, libsuperlu_dist, etc) and for simple configure systems -L%s -larpack' % spec['arpac'].prefix.lib will do. For complicated cases like trilinos and oce, which ave tens (!) of libraries, dependents (i.e. dealii) know which libs to pick up from a prefix. So using this class is an overkill and hard to maintain as the list of libraries may change between versions.

So i would not adopt this approach to every package, but only to a few virtuals (fftw is probably the only one missing).

What should be systematically addressed is lib folder. I don't have a good overview, but i guess libraires may end up both in prefix/lib and prefix/lib64 on 64bit systems. In some packages %s/lib is hardcoded, in others it is used as spec[package].prefix.lib, but I am not sure that each package somehow checks whether or not things are installed in lib or lib64. If that's the case already, then spec[package].prefix.lib should be used throughout.

@tgamblin p.s. maybe you copy-paste your comment into a separate issue to continue the discussion?

@alalazo @tgamblin I just updated to the latest develop and now I'm seeing infinite error messages:

Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.AttributeError'> ignored

If I checkout the commit before this PR was merged, everything works fine. There must be some infinite recursion going on here.

To reproduce the bug, just try installing any package. Even ones that don't depend on blas/lapack/etc. Maybe a getattribute problem?