From efd2a957810511589fc135e10b1bf9aada03c2eb Mon Sep 17 00:00:00 2001 From: alalazo Date: Thu, 3 Aug 2017 17:05:49 +0200 Subject: [PATCH] find has been changed to accept glob expressions Following the discussion with Todd and Adam, find has been modified to accept glob expressions. This should not affect performance as every glob implementation I inspected has 3 cases (no wildcard, wildcard but no directories involved, wildcard and directories involved) and uses fnmatch underneath. Mixins have been changed to do by default a non-recursive search (but a recursive search can still be triggered using the recursive keyword). --- lib/spack/llnl/util/filesystem.py | 41 +++++++------ lib/spack/spack/mixins.py | 11 +++- lib/spack/spack/spec.py | 10 +-- .../test/data/directory_search/a/foobar.txt | 0 .../test/data/directory_search/b/bar.txp | 0 .../test/data/directory_search/c/bar.txt | 0 lib/spack/spack/test/llnl/util/file_list.py | 61 +++++++++++++------ .../repos/builtin/packages/openmpi/package.py | 18 +----- 8 files changed, 80 insertions(+), 61 deletions(-) create mode 100644 lib/spack/spack/test/data/directory_search/a/foobar.txt create mode 100644 lib/spack/spack/test/data/directory_search/b/bar.txp create mode 100644 lib/spack/spack/test/data/directory_search/c/bar.txt diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index f8d77e2727..6b92a374c7 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -26,7 +26,6 @@ import errno import hashlib import fileinput -import fnmatch import glob import numbers import os @@ -606,7 +605,7 @@ def fix_darwin_install_name(path): break -def find(root, files, recurse=True): +def find(root, files, recursive=True): """Search for ``files`` starting from the ``root`` directory. Like GNU/BSD find but written entirely in Python. @@ -627,7 +626,7 @@ def find(root, files, recurse=True): is equivalent to: - >>> find('/usr/local/bin', 'python', recurse=False) + >>> find('/usr/local/bin', 'python', recursive=False) Accepts any glob characters accepted by fnmatch: @@ -652,7 +651,7 @@ def find(root, files, recurse=True): if isinstance(files, six.string_types): files = [files] - if recurse: + if recursive: return _find_recursive(root, files) else: return _find_non_recursive(root, files) @@ -666,11 +665,14 @@ def _find_recursive(root, search_files): # found in a key, and reconstructing the stable order later. found_files = collections.defaultdict(list) + # Make the path absolute to have os.walk also return an absolute path + root = os.path.abspath(root) + for path, _, list_files in os.walk(root): for search_file in search_files: - for list_file in list_files: - if fnmatch.fnmatch(list_file, search_file): - found_files[search_file].append(join_path(path, list_file)) + matches = glob.glob(os.path.join(path, search_file)) + matches = [os.path.join(path, x) for x in matches] + found_files[search_file].extend(matches) answer = [] for search_file in search_files: @@ -684,10 +686,13 @@ def _find_non_recursive(root, search_files): # can return files in any order (does not preserve stability) found_files = collections.defaultdict(list) - for list_file in os.listdir(root): - for search_file in search_files: - if fnmatch.fnmatch(list_file, search_file): - found_files[search_file].append(join_path(root, list_file)) + # Make the path absolute to have absolute path returned + root = os.path.abspath(root) + + for search_file in search_files: + matches = glob.glob(os.path.join(root, search_file)) + matches = [os.path.join(root, x) for x in matches] + found_files[search_file].extend(matches) answer = [] for search_file in search_files: @@ -878,7 +883,7 @@ def add_macro(self, macro): self._macro_definitions.append(macro) -def find_headers(headers, root, recurse=False): +def find_headers(headers, root, recursive=False): """Returns an iterable object containing a list of full paths to headers if found. @@ -896,7 +901,7 @@ def find_headers(headers, root, recurse=False): Parameters: headers (str or list of str): Header name(s) to search for root (str): The root directory to start searching from - recurses (bool, optional): if False search only root folder, + recursive (bool, optional): if False search only root folder, if True descends top-down from the root. Defaults to False. Returns: @@ -916,7 +921,7 @@ def find_headers(headers, root, recurse=False): # List of headers we are searching with suffixes headers = ['{0}.{1}'.format(header, suffix) for header in headers] - return HeaderList(find(root, headers, recurse)) + return HeaderList(find(root, headers, recursive)) class LibraryList(FileList): @@ -1057,7 +1062,7 @@ def find_system_libraries(libraries, shared=True): for library in libraries: for root in search_locations: - result = find_libraries(library, root, shared, recurse=True) + result = find_libraries(library, root, shared, recursive=True) if result: libraries_found += result break @@ -1065,7 +1070,7 @@ def find_system_libraries(libraries, shared=True): return libraries_found -def find_libraries(libraries, root, shared=True, recurse=False): +def find_libraries(libraries, root, shared=True, recursive=False): """Returns an iterable of full paths to libraries found in a root dir. Accepts any glob characters accepted by fnmatch: @@ -1084,7 +1089,7 @@ def find_libraries(libraries, root, shared=True, recurse=False): root (str): The root directory to start searching from shared (bool, optional): if True searches for shared libraries, otherwise for static. Defaults to True. - recurse (bool, optional): if False search only root folder, + recursive (bool, optional): if False search only root folder, if True descends top-down from the root. Defaults to False. Returns: @@ -1106,4 +1111,4 @@ def find_libraries(libraries, root, shared=True, recurse=False): # List of libraries we are searching with suffixes libraries = ['{0}.{1}'.format(lib, suffix) for lib in libraries] - return LibraryList(find(root, libraries, recurse)) + return LibraryList(find(root, libraries, recursive)) diff --git a/lib/spack/spack/mixins.py b/lib/spack/spack/mixins.py index 0a43e76961..d16f572a4e 100644 --- a/lib/spack/spack/mixins.py +++ b/lib/spack/spack/mixins.py @@ -163,6 +163,11 @@ def filter_compiler_wrappers(*files, **kwargs): these two keyword arguments, if present, will be forwarded to ``filter_file`` (see its documentation for more information on their behavior) + + recursive + this keyword argument, if present, will be forwarded to + ``find`` (see its documentation for more information on the + behavior) """ after = kwargs.get('after', 'install') relative_root = kwargs.get('relative_root', None) @@ -173,6 +178,10 @@ def filter_compiler_wrappers(*files, **kwargs): 'string': True } + find_kwargs = { + 'recursive': kwargs.get('recursive', False) + } + def _filter_compiler_wrappers_impl(self): # Compute the absolute path of the search root root = os.path.join( @@ -181,7 +190,7 @@ def _filter_compiler_wrappers_impl(self): # Compute the absolute path of the files to be filtered and # remove links from the list. - abs_files = llnl.util.filesystem.find(root, files) + abs_files = llnl.util.filesystem.find(root, files, **find_kwargs) abs_files = [x for x in abs_files if not os.path.islink(x)] x = llnl.util.filesystem.FileFilter(*abs_files) diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 61f127a66c..341e266493 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -689,7 +689,7 @@ def _headers_default_handler(descriptor, spec, cls): Raises: RuntimeError: If no headers are found """ - headers = find_headers('*', root=spec.prefix.include, recurse=True) + headers = find_headers('*', root=spec.prefix.include, recursive=True) if headers: return headers @@ -731,22 +731,22 @@ def _libs_default_handler(descriptor, spec, cls): if '+shared' in spec: libs = find_libraries( - name, root=spec.prefix, shared=True, recurse=True + name, root=spec.prefix, shared=True, recursive=True ) elif '~shared' in spec: libs = find_libraries( - name, root=spec.prefix, shared=False, recurse=True + name, root=spec.prefix, shared=False, recursive=True ) else: # Prefer shared libs = find_libraries( - name, root=spec.prefix, shared=True, recurse=True + name, root=spec.prefix, shared=True, recursive=True ) if libs: return libs libs = find_libraries( - name, root=spec.prefix, shared=False, recurse=True + name, root=spec.prefix, shared=False, recursive=True ) if libs: diff --git a/lib/spack/spack/test/data/directory_search/a/foobar.txt b/lib/spack/spack/test/data/directory_search/a/foobar.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lib/spack/spack/test/data/directory_search/b/bar.txp b/lib/spack/spack/test/data/directory_search/b/bar.txp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lib/spack/spack/test/data/directory_search/c/bar.txt b/lib/spack/spack/test/data/directory_search/c/bar.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lib/spack/spack/test/llnl/util/file_list.py b/lib/spack/spack/test/llnl/util/file_list.py index f7d5dde4d3..d92f15c5e1 100644 --- a/lib/spack/spack/test/llnl/util/file_list.py +++ b/lib/spack/spack/test/llnl/util/file_list.py @@ -30,7 +30,7 @@ import six import spack from llnl.util.filesystem import LibraryList, HeaderList -from llnl.util.filesystem import find_libraries, find_headers +from llnl.util.filesystem import find_libraries, find_headers, find @pytest.fixture() @@ -215,36 +215,40 @@ def test_add(self, header_list): @pytest.mark.parametrize('search_fn,search_list,root,kwargs', [ - (find_libraries, 'liba', search_dir, {'recurse': True}), - (find_libraries, ['liba'], search_dir, {'recurse': True}), - (find_libraries, 'libb', search_dir, {'recurse': True}), - (find_libraries, ['libc'], search_dir, {'recurse': True}), - (find_libraries, ['libc', 'liba'], search_dir, {'recurse': True}), - (find_libraries, ['liba', 'libc'], search_dir, {'recurse': True}), - (find_libraries, ['libc', 'libb', 'liba'], search_dir, {'recurse': True}), - (find_libraries, ['liba', 'libc'], search_dir, {'recurse': True}), + (find_libraries, 'liba', search_dir, {'recursive': True}), + (find_libraries, ['liba'], search_dir, {'recursive': True}), + (find_libraries, 'libb', search_dir, {'recursive': True}), + (find_libraries, ['libc'], search_dir, {'recursive': True}), + (find_libraries, ['libc', 'liba'], search_dir, {'recursive': True}), + (find_libraries, ['liba', 'libc'], search_dir, {'recursive': True}), (find_libraries, ['libc', 'libb', 'liba'], search_dir, - {'recurse': True, 'shared': False} + {'recursive': True} ), - (find_headers, 'a', search_dir, {'recurse': True}), - (find_headers, ['a'], search_dir, {'recurse': True}), - (find_headers, 'b', search_dir, {'recurse': True}), - (find_headers, ['c'], search_dir, {'recurse': True}), - (find_headers, ['c', 'a'], search_dir, {'recurse': True}), - (find_headers, ['a', 'c'], search_dir, {'recurse': True}), - (find_headers, ['c', 'b', 'a'], search_dir, {'recurse': True}), - (find_headers, ['a', 'c'], search_dir, {'recurse': True}), + (find_libraries, ['liba', 'libc'], search_dir, {'recursive': True}), + (find_libraries, + ['libc', 'libb', 'liba'], + search_dir, + {'recursive': True, 'shared': False} + ), + (find_headers, 'a', search_dir, {'recursive': True}), + (find_headers, ['a'], search_dir, {'recursive': True}), + (find_headers, 'b', search_dir, {'recursive': True}), + (find_headers, ['c'], search_dir, {'recursive': True}), + (find_headers, ['c', 'a'], search_dir, {'recursive': True}), + (find_headers, ['a', 'c'], search_dir, {'recursive': True}), + (find_headers, ['c', 'b', 'a'], search_dir, {'recursive': True}), + (find_headers, ['a', 'c'], search_dir, {'recursive': True}), (find_libraries, ['liba', 'libd'], os.path.join(search_dir, 'b'), - {'recurse': False} + {'recursive': False} ), (find_headers, ['b', 'd'], os.path.join(search_dir, 'b'), - {'recurse': False} + {'recursive': False} ), ]) def test_searching_order(search_fn, search_list, root, kwargs): @@ -275,3 +279,20 @@ def test_searching_order(search_fn, search_list, root, kwargs): # List should be empty here assert len(L) == 0 + + +@pytest.mark.parametrize('root,search_list,kwargs,expected', [ + (search_dir, '*/*bar.tx?', {'recursive': False}, [ + os.path.join(search_dir, 'a/foobar.txt'), + os.path.join(search_dir, 'b/bar.txp'), + os.path.join(search_dir, 'c/bar.txt'), + ]), + (search_dir, '*/*bar.tx?', {'recursive': True}, [ + os.path.join(search_dir, 'a/foobar.txt'), + os.path.join(search_dir, 'b/bar.txp'), + os.path.join(search_dir, 'c/bar.txt'), + ]) +]) +def test_find_with_globbing(root, search_list, kwargs, expected): + matches = find(root, search_list, **kwargs) + assert matches == expected diff --git a/var/spack/repos/builtin/packages/openmpi/package.py b/var/spack/repos/builtin/packages/openmpi/package.py index 889455aae0..3919863558 100644 --- a/var/spack/repos/builtin/packages/openmpi/package.py +++ b/var/spack/repos/builtin/packages/openmpi/package.py @@ -214,23 +214,7 @@ class Openmpi(AutotoolsPackage): conflicts('fabrics=pmi', when='@:1.5.4') # PMI support was added in 1.5.5 conflicts('fabrics=mxm', when='@:1.5.3') # MXM support was added in 1.5.4 - filter_compiler_wrappers( - 'mpicc-vt-wrapper-data.txt', - 'mpicc-wrapper-data.txt', - 'ortecc-wrapper-data.txt', - 'shmemcc-wrapper-data.txt', - 'mpic++-vt-wrapper-data.txt', - 'mpic++-wrapper-data.txt', - 'ortec++-wrapper-data.txt', - 'mpifort-vt-wrapper-data.txt', - 'mpifort-wrapper-data.txt', - 'shmemfort-wrapper-data.txt', - 'mpif90-vt-wrapper-data.txt', - 'mpif90-wrapper-data.txt', - 'mpif77-vt-wrapper-data.txt', - 'mpif77-wrapper-data.txt', - relative_root=os.path.join('share', 'openmpi') - ) + filter_compiler_wrappers('openmpi/*-wrapper-data*', relative_root='share') def url_for_version(self, version): url = "http://www.open-mpi.org/software/ompi/v{0}/downloads/openmpi-{1}.tar.bz2"