Restore computation of include directories

lib/spack/env/cc

@@ -410,10 +410,10 @@ case "$mode" in
     ld|ccld)
         # Set extra RPATHs
         IFS=':' read -ra extra_rpaths <<< "$SPACK_COMPILER_EXTRA_RPATHS"
-        for extra_rpath in "${extra_rpaths[@]}"; do
-            $add_rpaths && rpaths+=("$extra_rpath")
-            libdirs+=("$extra_rpath")
-        done
+        libdirs+=("${extra_rpaths[@]}")
+        if [[ "$add_rpaths" != "false" ]] ; then
+            rpaths+=("${extra_rpaths[@]}")
+        fi
 
         # Add SPACK_LDLIBS to args
         for lib in "${SPACK_LDLIBS[@]}"; do

lib/spack/llnl/util/filesystem.py

@@ -36,6 +36,7 @@
     'filter_file',
     'find',
     'find_headers',
+    'find_all_headers',
     'find_libraries',
     'find_system_libraries',
     'fix_darwin_install_name',
@@ -956,10 +957,46 @@ class HeaderList(FileList):
     commonly used compiler flags or names.
     """
 
+    include_regex = re.compile(r'(.*)(include)(.*)')
+
     def __init__(self, files):
         super(HeaderList, self).__init__(files)
 
         self._macro_definitions = []
+        self._directories = None
+
+    @property
+    def directories(self):
+        """Directories to be searched for header files."""
+        values = self._directories
+        if values is None:
+            values = self._default_directories()
+        return list(dedupe(values))
+
+    @directories.setter
+    def directories(self, value):
+        value = value or []
+        # Accept a single directory as input
+        if isinstance(value, six.string_types):
+            value = [value]
+
+        # Setting the property to None makes the initial default
+        # kick-in again
+        self._directories = [os.path.normpath(x) for x in value]
+
+    def _default_directories(self):
+        """Default computation of directories based on the list of
+        header files.
+        """
+        dir_list = super(HeaderList, self).directories
+        values = []
+        for d in dir_list:
+            # If the path contains a subdirectory named 'include' then stop
+            # there and don't add anything else to the path.
+            m = self.include_regex.match(d)
+            value = os.path.join(*m.group(1, 2)) if m else d
+            values.append(value)
+        return values
 
     @property
     def headers(self):
@@ -1095,6 +1132,19 @@ def find_headers(headers, root, recursive=False):
     return HeaderList(find(root, headers, recursive))
 
 
+def find_all_headers(root):
+    """Convenience function that returns the list of all headers found
+    in the directory passed as argument.
+
+    Args:
+        root (path): directory where to look recursively for header files
+
+    Returns:
+        List of all headers found in ``root`` and subdirectories.
+    """
+    return find_headers('*', root=root, recursive=True)
+
+
 class LibraryList(FileList):
     """Sequence of absolute paths to libraries
 

lib/spack/spack/build_environment.py

@@ -292,13 +292,10 @@ def set_build_environment_variables(pkg, env, dirty):
         if dep in rpath_deps:
             rpath_dirs.extend(dep_link_dirs)
 
-        # TODO: fix the line below, currently the logic is broken for
-        # TODO: packages that uses directories as namespaces e.g.
-        # TODO: #include <boost/xxx.hpp>
-        # include_dirs.extend(query.headers.directories)
-
-        if os.path.isdir(dep.prefix.include):
-            include_dirs.append(dep.prefix.include)
+        try:
+            include_dirs.extend(query.headers.directories)
+        except spack.spec.NoHeadersError:
+            tty.debug("No headers found for {0}".format(dep.name))
 
     link_dirs = list(dedupe(filter_system_paths(link_dirs)))
     include_dirs = list(dedupe(filter_system_paths(include_dirs)))

lib/spack/spack/test/build_environment.py

@@ -14,7 +14,7 @@
 from spack.util.spack_yaml import syaml_dict, syaml_str
 from spack.util.environment import EnvironmentModifications
 
-from llnl.util.filesystem import LibraryList, HeaderList
+from llnl.util.filesystem import LibraryList
 
 
 @pytest.fixture
@@ -221,7 +221,9 @@ def test_package_inheritance_module_setup(config, mock_packages):
 
 
 def test_set_build_environment_variables(
-        config, mock_packages, working_env, monkeypatch, tmpdir_factory):
+        config, mock_packages, working_env, monkeypatch,
+        installation_dir_with_headers
+):
     """Check that build_environment supplies the needed library/include
     directories via the SPACK_LINK_DIRS and SPACK_INCLUDE_DIRS environment
     variables.
@@ -238,15 +240,10 @@ def test_set_build_environment_variables(
     dep_lib_dirs = ['/test/path/to', '/test/path/to/subdir']
     dep_libs = LibraryList(dep_lib_paths)
 
-    dep2_prefix = tmpdir_factory.mktemp('prefix')
-    dep2_include = dep2_prefix.ensure('include', dir=True)
     dep2_pkg = root['dt-diamond-right'].package
-    dep2_pkg.spec.prefix = str(dep2_prefix)
-    dep2_inc_paths = ['/test2/path/to/ex1.h', '/test2/path/to/subdir/ex2.h']
-    dep2_includes = HeaderList(dep2_inc_paths)
+    dep2_pkg.spec.prefix = str(installation_dir_with_headers)
 
     setattr(dep_pkg, 'libs', dep_libs)
-    setattr(dep2_pkg, 'headers', dep2_includes)
     try:
         pkg = root.package
         env_mods = EnvironmentModifications()
@@ -271,11 +268,16 @@ def normpaths(paths):
             normpaths(root_libdirs + dep_lib_dirs))
 
         header_dir_var = os.environ['SPACK_INCLUDE_DIRS']
-        # As long as a dependency package has an 'include' prefix, it is added
-        # (regardless of whether it contains any header files)
-        assert (
-            normpaths(header_dir_var.split(':')) ==
-            normpaths([str(dep2_include)]))
+
+        # The default implementation looks for header files only
+        # in <prefix>/include and subdirectories
+        prefix = str(installation_dir_with_headers)
+        include_dirs = normpaths(header_dir_var.split(':'))
+
+        assert os.path.join(prefix, 'include') in include_dirs
+        assert os.path.join(prefix, 'include', 'boost') not in include_dirs
+        assert os.path.join(prefix, 'path', 'to') not in include_dirs
+        assert os.path.join(prefix, 'path', 'to', 'subdir') not in include_dirs
+
     finally:
         delattr(dep_pkg, 'libs')
-        delattr(dep2_pkg, 'headers')

lib/spack/spack/test/conftest.py

@@ -716,6 +716,35 @@ def mutable_mock_env_path(tmpdir_factory):
     spack.environment.env_path = saved_path
 
 
+@pytest.fixture()
+def installation_dir_with_headers(tmpdir_factory):
+    """Mock installation tree with a few headers placed in different
+    subdirectories. Shouldn't be modified by tests as it is session
+    scoped.
+    """
+    root = tmpdir_factory.mktemp('prefix')
+
+    # Create a few header files:
+    #
+    # <prefix>
+    # |-- include
+    # |   |--boost
+    # |   |   |-- ex3.h
+    # |   |-- ex3.h
+    # |-- path
+    #     |-- to
+    #         |-- ex1.h
+    #         |-- subdir
+    #             |-- ex2.h
+    #
+    root.ensure('include', 'boost', 'ex3.h')
+    root.ensure('include', 'ex3.h')
+    root.ensure('path', 'to', 'ex1.h')
+    root.ensure('path', 'to', 'subdir', 'ex2.h')
+
+    return root
+
+
 ##########
 # Mock packages
 ##########

lib/spack/spack/test/llnl/util/filesystem.py

@@ -215,3 +215,69 @@ def test_move_transaction_rollback(tmpdir):
         pass
 
     assert h == fs.hash_directory(str(tmpdir))
+
+
+@pytest.mark.regression('10601')
+@pytest.mark.regression('10603')
+def test_recursive_search_of_headers_from_prefix(
+        installation_dir_with_headers
+):
+    # Try to inspect recursively from <prefix> and ensure we don't get
+    # subdirectories of the '<prefix>/include' path
+    prefix = str(installation_dir_with_headers)
+    header_list = fs.find_all_headers(prefix)
+
+    # Check that the header files we expect are all listed
+    assert os.path.join(prefix, 'include', 'ex3.h') in header_list
+    assert os.path.join(prefix, 'include', 'boost', 'ex3.h') in header_list
+    assert os.path.join(prefix, 'path', 'to', 'ex1.h') in header_list
+    assert os.path.join(prefix, 'path', 'to', 'subdir', 'ex2.h') in header_list
+
+    # Check that when computing directories we exclude <prefix>/include/boost
+    include_dirs = header_list.directories
+    assert os.path.join(prefix, 'include') in include_dirs
+    assert os.path.join(prefix, 'include', 'boost') not in include_dirs
+    assert os.path.join(prefix, 'path', 'to') in include_dirs
+    assert os.path.join(prefix, 'path', 'to', 'subdir') in include_dirs
+
+
+@pytest.mark.parametrize('list_of_headers,expected_directories', [
+    (['/pfx/include/foo.h', '/pfx/include/subdir/foo.h'], ['/pfx/include']),
+    (['/pfx/include/foo.h', '/pfx/subdir/foo.h'],
+     ['/pfx/include', '/pfx/subdir']),
+    (['/pfx/include/subdir/foo.h', '/pfx/subdir/foo.h'],
+     ['/pfx/include', '/pfx/subdir'])
+])
+def test_computation_of_header_directories(
+        list_of_headers, expected_directories
+):
+    hl = fs.HeaderList(list_of_headers)
+    assert hl.directories == expected_directories
+
+
+def test_headers_directory_setter():
+    hl = fs.HeaderList(
+        ['/pfx/include/subdir/foo.h', '/pfx/include/subdir/bar.h']
+    )
+
+    # Set directories using a list
+    hl.directories = ['/pfx/include/subdir']
+    assert hl.directories == ['/pfx/include/subdir']
+
+    # If it's a single directory it's fine to not wrap it into a list
+    # when setting the property
+    hl.directories = '/pfx/include/subdir'
+    assert hl.directories == ['/pfx/include/subdir']
+
+    # Paths are normalized, so it doesn't matter how many backslashes etc.
+    # are present in the original directory being used
+    hl.directories = '/pfx/include//subdir/'
+    assert hl.directories == ['/pfx/include/subdir']
+
+    # Setting an empty list is allowed and returns an empty list
+    hl.directories = []
+    assert hl.directories == []
+
+    # Setting directories to None also returns an empty list
+    hl.directories = None
+    assert hl.directories == []

var/spack/repos/builtin/packages/libxml2/package.py

@@ -36,6 +36,13 @@ class Libxml2(AutotoolsPackage):
     resource(name='xmlts', url='http://www.w3.org/XML/Test/xmlts20080827.tar.gz',
              sha256='96151685cec997e1f9f3387e3626d61e6284d4d6e66e0e440c209286c03e9cc7')
 
+    @property
+    def headers(self):
+        include_dir = self.spec.prefix.include.libxml2
+        hl = find_all_headers(include_dir)
+        hl.directories = include_dir
+        return hl
+
     def configure_args(self):
         spec = self.spec
 
@@ -51,10 +58,6 @@ def configure_args(self):
 
         return args
 
-    def setup_dependent_environment(self, spack_env, run_env, dependent_spec):
-        spack_env.prepend_path('CPATH', self.prefix.include.libxml2)
-        run_env.prepend_path('CPATH', self.prefix.include.libxml2)
-
     @run_after('install')
     @on_package_attributes(run_tests=True)
     def import_module_test(self):