Skip to content

generic sanity-checking of libraries #1424

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.

Sign up for GitHub

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

Closed
torbjoernk wants to merge 8 commits into from
Closed

generic sanity-checking of libraries #1424

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
403f0c8
use correct sanity check function
torbjoernk Oct 12, 2015
a17eb36
extend sanity_check_paths for libraries
torbjoernk Oct 12, 2015
ac06c32
change specification of libraries
torbjoernk Oct 13, 2015
e17539d
Merge branch 'develop' into feature/sanity-check-libs
torbjoernk Oct 29, 2015
cbdaa2d
Merge branch 'develop' into feature/sanity-check-libs
torbjoernk Nov 27, 2015
fb22cd4
only deal with single library suffix per lib type
torbjoernk Nov 27, 2015
c9d153e
test: new easyblock test for sanity_check_paths
torbjoernk Nov 27, 2015
6ee88ee
fix test cases
torbjoernk Jan 25, 2016
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
24 changes: 23 additions & 1 deletion easybuild/framework/easyblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1546,6 +1546,10 @@ def sanity_check_step(self, custom_paths=None, custom_commands=None, extension=F
- if *any* of the files/subdirectories in the installation directory listed
in sanity_check_paths are non-existent (or empty), the sanity check fails
"""
# constants for library checking (TODO: should probably be defined somewhere else/configurab;e)
LIB_DIRS = ['lib', 'lib64']
LIB_SUFFIXES = {'static': ['.a'], 'shared': ['.so', '.dyn']}
Copy link
Member

@boegel boegel Oct 12, 2015

Choose a reason for hiding this comment

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

constant should be defined at the top of this file, below imports

rather than hardcoding .so or .dyn, use get_shared_lib_ext() from easybuild.tools.systemtools

Copy link
Author

@torbjoernk torbjoernk Oct 13, 2015

Choose a reason for hiding this comment

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

Awesome. I was looking for such a method but couldn't find it. Thanks.

Edit: There is no such method for static libraries, right?


# supported/required keys in for sanity check paths, along with function used to check the paths
path_keys_and_check = {
'files': lambda fp: os.path.exists(fp), # files must exist
Expand All @@ -1566,6 +1570,24 @@ def sanity_check_step(self, custom_paths=None, custom_commands=None, extension=F
else:
self.log.info("Using specified sanity check paths: %s" % paths)

# transform specified libs into simple file paths
if 'libs' in paths.keys():
for lib in paths['libs']:
Copy link
Member

@boegel boegel Oct 12, 2015

Choose a reason for hiding this comment

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

use for lib in paths.get('libs', []) to avoid deep indentation

if 'name' in lib.keys() and 'kind' in lib.keys():
if lib['kind'] in LIB_SUFFIXES.keys():
# TODO: also check possible library version numbers (e.g. libawesome.so.3.0.1)
libpaths = ()
for dir in LIB_DIRS:
for suffix in LIB_SUFFIXES[lib['kind']]:
libpaths += (os.path.join(dir, lib['name'] + suffix), )
Copy link
Member

@boegel boegel Oct 12, 2015

Choose a reason for hiding this comment

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

use a list for libpaths, which is easier to update, and then use tuple(libpaths) below

paths['files'] += [libpaths]
Copy link
Member

@boegel boegel Oct 12, 2015

Choose a reason for hiding this comment

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

use:

paths['files'].append(tuple(libpaths))

else:
raise EasyBuildError("Library kind must either be static or shared.")
Copy link
Member

@boegel boegel Oct 12, 2015

Choose a reason for hiding this comment

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

don't hardcode the available keys in the message, use LIB_SUFFIXES.keys():

raise EasyBuildError("Unknown kind of library, only known kinds are: %s" % ' '.join(LIB_SUFFIXES.keys()))

else:
raise EasyBuildError("Libraries are specified via 'name' and 'kind'.")
Copy link
Member

@boegel boegel Oct 12, 2015

Choose a reason for hiding this comment

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

s/are/must be/

# remove the libs
del paths['libs']
Copy link
Member

@boegel boegel Oct 12, 2015

Choose a reason for hiding this comment

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

why does this need to be removed?

Copy link
Author

@torbjoernk torbjoernk Oct 13, 2015

Choose a reason for hiding this comment

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

Otherwise the following exception will be thrown:

if not ks == req_keys or sum(valnottypes) > 0 or sum(lenvals) == 0:
  raise EasyBuildError("Incorrect format for sanity_check_paths (should (only) have %s keys, "
                       "values should be lists (at least one non-empty)).", ','.join(req_keys))

This might be circumvented by implementing the library checks as a lambda in path_keys_and_check (similar for files and dirs) though that would make it impossible to treat the libraries as regular files (and reusing the logic for these).


# check sanity check paths
ks = sorted(paths.keys())
valnottypes = [not isinstance(x, list) for x in paths.values()]
Expand All @@ -1585,7 +1607,7 @@ def sanity_check_step(self, custom_paths=None, custom_commands=None, extension=F
found = False
for name in xs:
path = os.path.join(self.installdir, name)
if os.path.exists(path):
if check_fn(path):
Copy link
Member

@boegel boegel Oct 12, 2015

Choose a reason for hiding this comment

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

this is indeed a bug, this fix is also included in #1388 (which doesn't mean it shouldn't be included here)

maybe we should make this a separate PR though, to make the bug fix stand out; it may have some fallout, i.e. causing sanity checks to fail on empty directories where they didn't before (they should have though)

Copy link
Member

@boegel boegel Oct 21, 2015

Choose a reason for hiding this comment

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

even worse, this check will now pass on directories that are actually files

this distinction can be important, for example if you want to make sure that a Python package is installed as a zipped egg, i.e., that the .egg is a file rather than a directory

so, we really need a separate PR for this

Copy link
Member

@boegel boegel Oct 21, 2015

Choose a reason for hiding this comment

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

see #1436

self.log.debug("Sanity check: found %s %s in %s" % (key[:-1], name, self.installdir))
found = True
break
Expand Down