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

Reworking of lapack_shared_libs and similar properties #1682

Merged
merged 8 commits into from
Sep 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions lib/spack/docs/packaging_guide.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2090,12 +2090,11 @@ Blas and Lapack libraries

Different packages provide implementation of ``Blas`` and ``Lapack`` routines.
The names of the resulting static and/or shared libraries differ from package
to package. In order to make the ``install()`` method indifferent to the
to package. In order to make the ``install()`` method independent of the
choice of ``Blas`` implementation, each package which provides it
sets up ``self.spec.blas_shared_lib`` and ``self.spec.blas_static_lib`` to
point to the shared and static ``Blas`` libraries, respectively. The same
applies to packages which provide ``Lapack``. Package developers are advised to
use these variables, for example ``spec['blas'].blas_shared_lib`` instead of
sets up ``self.spec.blas_libs`` to point to the correct ``Blas`` libraries.
The same applies to packages which provide ``Lapack``. Package developers are advised to
Copy link
Member

Choose a reason for hiding this comment

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

Blas-->BLAS

use these variables, for example ``spec['blas'].blas_libs.joined()`` instead of
Copy link
Member

Choose a reason for hiding this comment

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

Lapack-->LAPACK

hard-coding ``join_path(spec['blas'].prefix.lib, 'libopenblas.so')``.

^^^^^^^^^^^^^^^^^^^^^
Expand Down
197 changes: 164 additions & 33 deletions lib/spack/llnl/util/filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,22 @@
# License along with this program; if not, write to the Free Software
# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
##############################################################################
import os
import collections
import errno
import fileinput
import getpass
import glob
import numbers
import os
import re
import shutil
import stat
import errno
import getpass
from contextlib import contextmanager
import subprocess
import fileinput
import sys
from contextlib import contextmanager

import llnl.util.tty as tty
from llnl.util.lang import dedupe

__all__ = ['set_install_permissions', 'install', 'install_tree',
'traverse_tree',
Expand All @@ -42,8 +46,8 @@
'filter_file',
'FileFilter', 'change_sed_delimiter', 'is_exe', 'force_symlink',
'set_executable', 'copy_mode', 'unset_executable_mode',
'remove_dead_links', 'remove_linked_tree', 'find_library_path',
'fix_darwin_install_name', 'to_link_flags', 'to_lib_name']
'remove_dead_links', 'remove_linked_tree',
'fix_darwin_install_name', 'find_libraries', 'LibraryList']


def filter_file(regex, repl, *filenames, **kwargs):
Expand Down Expand Up @@ -326,7 +330,7 @@ def traverse_tree(source_root, dest_root, rel_path='', **kwargs):
follow_links = kwargs.get('follow_link', False)

# Yield in pre or post order?
order = kwargs.get('order', 'pre')
order = kwargs.get('order', 'pre')
if order not in ('pre', 'post'):
raise ValueError("Order must be 'pre' or 'post'.")

Expand All @@ -338,16 +342,16 @@ def traverse_tree(source_root, dest_root, rel_path='', **kwargs):
return

source_path = os.path.join(source_root, rel_path)
dest_path = os.path.join(dest_root, rel_path)
dest_path = os.path.join(dest_root, rel_path)

# preorder yields directories before children
if order == 'pre':
yield (source_path, dest_path)

for f in os.listdir(source_path):
source_child = os.path.join(source_path, f)
dest_child = os.path.join(dest_path, f)
rel_child = os.path.join(rel_path, f)
dest_child = os.path.join(dest_path, f)
rel_child = os.path.join(rel_path, f)

# Treat as a directory
if os.path.isdir(source_child) and (
Expand Down Expand Up @@ -440,35 +444,162 @@ def fix_darwin_install_name(path):
stdout=subprocess.PIPE).communicate()[0]
break

# Utilities for libraries


def to_lib_name(library):
"""Transforms a path to the library /path/to/lib<name>.xyz into <name>
class LibraryList(collections.Sequence):
"""Sequence of absolute paths to libraries

Provides a few convenience methods to manipulate library paths and get
commonly used compiler flags or names
"""
# Assume libXYZ.suffix
return os.path.basename(library)[3:].split(".")[0]

def __init__(self, libraries):
self.libraries = list(libraries)

def to_link_flags(library):
"""Transforms a path to a <library> into linking flags -L<dir> -l<name>.
@property
def directories(self):
Copy link
Member

Choose a reason for hiding this comment

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

donno what's the convention, but maybe one-line doc-string for properties? Especially for things like libnames vs names.

"""Stable de-duplication of the directories where the libraries
reside

Return:
A string of linking flags.
"""
dir = os.path.dirname(library)
name = to_lib_name(library)
res = '-L%s -l%s' % (dir, name)
return res
>>> l = LibraryList(['/dir1/liba.a', '/dir2/libb.a', '/dir1/libc.a'])
>>> assert l.directories == ['/dir1', '/dir2']
"""
return list(dedupe(
os.path.dirname(x) for x in self.libraries if os.path.dirname(x)
))

@property
def basenames(self):
"""Stable de-duplication of the base-names in the list

>>> l = LibraryList(['/dir1/liba.a', '/dir2/libb.a', '/dir3/liba.a'])
>>> assert l.basenames == ['liba.a', 'libb.a']
"""
return list(dedupe(os.path.basename(x) for x in self.libraries))

@property
def names(self):
Copy link
Member

Choose a reason for hiding this comment

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

matter of convention, but maybe call this one libname as (from here )

The soname has the prefix lib, the name of the library, the phrase .so, followed by a period and a version number that is incremented whenever the interface changes (as a special exception, the lowest-level C libraries don't start with lib).

in other words libFOO.so has a library name FOO.

One would need to figure out how to call the property above then.

Copy link
Member

Choose a reason for hiding this comment

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

ps. although i was not able to find a definite answer on this convention.

Copy link
Member

Choose a reason for hiding this comment

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

p.p.s. Now i see why you chose these names:

libnames is lib+names of libraries, whereas names are only names of the libraries.

I think that's the best one can do if you provide both properties accessible from class instantiations.

"""Stable de-duplication of library names in the list

>>> l = LibraryList(['/dir1/liba.a', '/dir2/libb.a', '/dir3/liba.so'])
>>> assert l.names == ['a', 'b']
"""
return list(dedupe(x.split('.')[0][3:] for x in self.basenames))

@property
def search_flags(self):
"""Search flags for the libraries

>>> l = LibraryList(['/dir1/liba.a', '/dir2/libb.a', '/dir1/liba.so'])
>>> assert l.search_flags == '-L/dir1 -L/dir2'
"""
return ' '.join(['-L' + x for x in self.directories])

@property
def link_flags(self):
"""Link flags for the libraries

>>> l = LibraryList(['/dir1/liba.a', '/dir2/libb.a', '/dir1/liba.so'])
>>> assert l.search_flags == '-la -lb'
"""
return ' '.join(['-l' + name for name in self.names])

@property
Copy link
Member

Choose a reason for hiding this comment

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

docstring missing

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it's necessary ? I thought the line below was quite self explanatory :

self.search_flags + ' ' + self.link_flags

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyhow, now it's there

Copy link
Member

Choose a reason for hiding this comment

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

for sure it is, just for consistency as you documented all properties but this one.

def ld_flags(self):
"""Search flags + link flags

def find_library_path(libname, *paths):
"""Searches for a file called <libname> in each path.
>>> l = LibraryList(['/dir1/liba.a', '/dir2/libb.a', '/dir1/liba.so'])
>>> assert l.search_flags == '-L/dir1 -L/dir2 -la -lb'
"""
return self.search_flags + ' ' + self.link_flags

Copy link
Member

Choose a reason for hiding this comment

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

i would add one more property for convenience: join what you call search_flags and link_flags into a single string -L/path/to/dir1 -L/path/to/dir2 -ld1 ld11 -ld2 -lbar -lbaz

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Return:
directory where the library was found, if found. None otherwise.
def __getitem__(self, item):
cls = type(self)
if isinstance(item, numbers.Integral):
return self.libraries[item]
return cls(self.libraries[item])

def __add__(self, other):
return LibraryList(dedupe(self.libraries + list(other)))

def __radd__(self, other):
return self.__add__(other)

def __eq__(self, other):
return self.libraries == other.libraries

def __len__(self):
return len(self.libraries)

def joined(self, separator=' '):
return separator.join(self.libraries)

def __repr__(self):
return self.__class__.__name__ + '(' + repr(self.libraries) + ')'

def __str__(self):
return self.joined()


def find_libraries(args, root, shared=True, recurse=False):
"""Returns an iterable object containing a list of full paths to
Copy link
Member

Choose a reason for hiding this comment

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

👍

libraries if found.

Args:
args: iterable object containing a list of library names to \
search for (e.g. 'libhdf5')
Copy link
Member

Choose a reason for hiding this comment

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

Can we allow this to take both 'libhdf5' and ['libhdf5'] as arguments? Most of the time, we will only be looking for a single library.

root: root folder where to start searching
shared: if True searches for shared libraries, otherwise for static
recurse: if False search only root folder, if True descends top-down \
from the root

Returns:
list of full paths to the libraries that have been found
"""
for path in paths:
library = join_path(path, libname)
if os.path.exists(library):
return path
return None
if not isinstance(args, collections.Sequence) or isinstance(args, str):
message = '{0} expects a sequence of strings as first argument'
message += ' [got {1} instead]'
raise TypeError(message.format(find_libraries.__name__, type(args)))

# Construct the right suffix for the library
if shared is True:
suffix = 'dylib' if sys.platform == 'darwin' else 'so'
else:
suffix = 'a'
# List of libraries we are searching with suffixes
libraries = ['{0}.{1}'.format(lib, suffix) for lib in args]
# Search method
if recurse is False:
search_method = _find_libraries_non_recursive
else:
search_method = _find_libraries_recursive

return search_method(libraries, root)


def _find_libraries_recursive(libraries, root):
Copy link
Member

Choose a reason for hiding this comment

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

maybe hide this one and next from from users inside class _Internal:, see http://stackoverflow.com/a/551048

i.e. make sure that inside packages only find_libraries(args, root, shared=True, recurse=False) can be called and the two functions are for internal usage only (similar to private for classes in C++ or Unnamed namespaces for utility functions)

Copy link
Member

@davydden davydden Aug 31, 2016

Choose a reason for hiding this comment

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

nevermind, i guess that's exactly how it works now as the two functions are not listed in __all__ above?

Copy link
Member Author

Choose a reason for hiding this comment

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

@davydden I missed this comment : in python there's no concept of private attribute, and everything is accessible. What __all__ does is to control what gets imported when you do something like :

from <module> import *

library_dict = collections.defaultdict(list)
for path, _, files in os.walk(root):
for lib in libraries:
if lib in files:
library_dict[lib].append(
join_path(path, lib)
)
answer = []
for lib in libraries:
answer.extend(library_dict[lib])
return LibraryList(answer)


def _find_libraries_non_recursive(libraries, root):

def lib_or_none(lib):
library = join_path(root, lib)
if not os.path.exists(library):
return None
return library

return LibraryList(
[lib_or_none(lib) for lib in libraries if lib_or_none(lib) is not None]
)
16 changes: 16 additions & 0 deletions lib/spack/llnl/util/lang.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,22 @@ def __iter__(self):
return wrapper()


def dedupe(sequence):
"""Yields a stable de-duplication of an hashable sequence

Args:
sequence: hashable sequence to be de-duplicated

Returns:
stable de-duplication of the sequence
"""
seen = set()
for x in sequence:
if x not in seen:
yield x
seen.add(x)
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this. How is this different from a regular set? Why not just call set() above instead of list(dedupe())?

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamjstewart The keyword here is "stable" : set doesn't give you any guarantee on the order of the elements, while dedupe preserves the original order. In the context of library linking this is quite important to resolve symbols correctly.



class RequiredAttributeError(ValueError):

def __init__(self, message):
Expand Down
7 changes: 7 additions & 0 deletions lib/spack/spack/spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,13 @@ def __init__(self, spec_like, *dep_like, **kwargs):
# XXX(deptype): default deptypes
self._add_dependency(spec, ('build', 'link'))

def __getattr__(self, item):
"""Delegate to self.package if the attribute is not in the spec"""
# This line is to avoid infinite recursion in case package is
# not present among self attributes
package = super(Spec, self).__getattribute__('package')
return getattr(package, item)
Copy link
Member

Choose a reason for hiding this comment

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

I've confirmed that these changes caused the bug I'm seeing. When I comment this function out, Spack works again. What was this added for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Today I succeeded installing cp2k before pushing changes and right now I correctly installed hdf5 on a fresh develop checkout... sorry I can't reproduce the problem...

The two lines are needed to forward requests for non-existing attributes from Spec to Package. It's what makes :

blas = spec['blas'].blas_libs

possible instead of :

blas = spec['blas'].package.blas_libs

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this is weird. Spack works on my laptop but not on our cluster. Going to investigate further.

Copy link
Member Author

@alalazo alalazo Sep 21, 2016

Choose a reason for hiding this comment

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

Ouch... could reproduce with python@2.6. I wonder why Travis never fired up a warning about that

EDIT : Travis seems to be running with 2.6.9 and no issues. Python 2.6.6 instead shows the problem.

Copy link
Member

Choose a reason for hiding this comment

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

On my cluster, Spack works with Python 2.7 but crashes with the system Python 2.6 Can you try 2.6 on your end to see if you can reproduce the problem?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm running Python 2.6.6 too.

Copy link
Member

Choose a reason for hiding this comment

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

@alalazo: if you're working on a fix I'll merge it asap, otherwise I'll work on it.

Copy link
Member

@tgamblin tgamblin Sep 21, 2016

Choose a reason for hiding this comment

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

EDIT : Travis seems to be running with 2.6.9 and no issues. Python 2.6.6 instead shows the problem.

If only we could just drop 2.6. Sigh.

Copy link
Member Author

@alalazo alalazo Sep 21, 2016

Choose a reason for hiding this comment

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

@tgamblin Sorry for the trouble. I couldn't find a solution that works for both python versions right now. If you couldn't solve it right away, feel free to revert the merge and I'll work it out without impacting everyone...

Copy link
Member Author

Choose a reason for hiding this comment

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

@tgamblin I think #1823 will solve this issue.


def get_dependency(self, name):
dep = self._dependencies.get(name)
if dep is not None:
Expand Down
1 change: 1 addition & 0 deletions lib/spack/spack/test/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
'git_fetch',
'hg_fetch',
'install',
'library_list',
'link_tree',
'lock',
'make_executable',
Expand Down
Loading