Fix #620, Resolve #664. Fix issues with build environment.

- Also added better regression tests for build environment.

lib/spack/spack/build_environment.py

@@ -213,7 +213,7 @@ def set_module_variables_for_package(pkg, module):
     # TODO: of build dependencies, as opposed to link dependencies.
     # TODO: Currently, everything is a link dependency, but tools like
     # TODO: this shouldn't be.
-    m.cmake = which("cmake")
+    m.cmake = Executable('cmake')
 
     # standard CMake arguments
     m.std_cmake_args = ['-DCMAKE_INSTALL_PREFIX=%s' % pkg.prefix,
@@ -278,21 +278,6 @@ def parent_class_modules(cls):
     return result
 
 
-def setup_module_variables_for_dag(pkg):
-    """Set module-scope variables for all packages in the DAG."""
-    for spec in pkg.spec.traverse(order='post'):
-        # If a user makes their own package repo, e.g.
-        # spack.repos.mystuff.libelf.Libelf, and they inherit from
-        # an existing class like spack.repos.original.libelf.Libelf,
-        # then set the module variables for both classes so the
-        # parent class can still use them if it gets called.
-        spkg = spec.package
-        modules = parent_class_modules(spkg.__class__)
-        for mod in modules:
-            set_module_variables_for_package(spkg, mod)
-        set_module_variables_for_package(spkg, spkg.module)
-
-
 def setup_package(pkg):
     """Execute all environment setup routines."""
     spack_env = EnvironmentModifications()
@@ -316,20 +301,26 @@ def setup_package(pkg):
 
     set_compiler_environment_variables(pkg, spack_env)
     set_build_environment_variables(pkg, spack_env)
-    setup_module_variables_for_dag(pkg)
+
+    # traverse in postorder so package can use vars from its dependencies
+    spec = pkg.spec
+    for dspec in pkg.spec.traverse(order='post'):
+        # If a user makes their own package repo, e.g.
+        # spack.repos.mystuff.libelf.Libelf, and they inherit from
+        # an existing class like spack.repos.original.libelf.Libelf,
+        # then set the module variables for both classes so the
+        # parent class can still use them if it gets called.
+        spkg = dspec.package
+        modules = parent_class_modules(spkg.__class__)
+        for mod in modules:
+            set_module_variables_for_package(spkg, mod)
+        set_module_variables_for_package(spkg, spkg.module)
 
         # Allow dependencies to modify the module
-    spec = pkg.spec
+        dpkg = dspec.package
-    for dependency_spec in spec.traverse(root=False):
-        dpkg = dependency_spec.package
         dpkg.setup_dependent_package(pkg.module, spec)
-
-    # Allow dependencies to set up environment as well
-    for dependency_spec in spec.traverse(root=False):
-        dpkg = dependency_spec.package
         dpkg.setup_dependent_environment(spack_env, run_env, spec)
 
-    # Allow the package to apply some settings.
     pkg.setup_environment(spack_env, run_env)
 
     # Make sure nothing's strange about the Spack environment.

lib/spack/spack/test/install.py

@@ -64,7 +64,14 @@ def tearDown(self):
         shutil.rmtree(self.tmpdir, ignore_errors=True)
 
 
-    def test_install_and_uninstall(self):
+    def fake_fetchify(self, pkg):
+        """Fake the URL for a package so it downloads from a file."""
+        fetcher = FetchStrategyComposite()
+        fetcher.append(URLFetchStrategy(self.repo.url))
+        pkg.fetcher = fetcher
+
+
+    def ztest_install_and_uninstall(self):
         # Get a basic concrete spec for the trivial install package.
         spec = Spec('trivial_install_test_package')
         spec.concretize()
@@ -73,11 +80,7 @@ def test_install_and_uninstall(self):
         # Get the package
         pkg = spack.repo.get(spec)
 
-        # Fake the URL for the package so it downloads from a file.
+        self.fake_fetchify(pkg)
-
-        fetcher = FetchStrategyComposite()
-        fetcher.append(URLFetchStrategy(self.repo.url))
-        pkg.fetcher = fetcher
 
         try:
             pkg.do_install()
@@ -85,3 +88,17 @@ def test_install_and_uninstall(self):
         except Exception, e:
             pkg.remove_prefix()
             raise
+
+
+    def test_install_environment(self):
+        spec = Spec('cmake-client').concretized()
+
+        for s in spec.traverse():
+            self.fake_fetchify(s.package)
+
+        pkg = spec.package
+        try:
+            pkg.do_install()
+        except Exception, e:
+            pkg.remove_prefix()
+            raise

var/spack/repos/builtin.mock/packages/cmake-client/package.py

@@ -0,0 +1,89 @@
+##############################################################################
+# Copyright (c) 2013, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Written by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/llnl/spack
+# Please also see the LICENSE file for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License (as published by
+# the Free Software Foundation) version 2.1 dated February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+import os
+
+def check(condition, msg):
+    """Raise an install error if condition is False."""
+    if not condition:
+        raise InstallError(msg)
+
+
+class CmakeClient(Package):
+    """A dumy package that uses cmake."""
+    homepage  = 'https://www.example.com'
+    url       = 'https://www.example.com/cmake-client-1.0.tar.gz'
+
+    version('1.0', '4cb3ff35b2472aae70f542116d616e63')
+
+    depends_on('cmake')
+
+
+    def setup_environment(self, spack_env, run_env):
+        spack_cc    # Ensure spack module-scope variable is avaiabl
+        check(from_cmake == "from_cmake",
+              "setup_environment couldn't read global set by cmake.")
+
+        check(self.spec['cmake'].link_arg == "test link arg",
+              "link arg on dependency spec not readable from setup_environment.")
+
+
+    def setup_dependent_environment(self, spack_env, run_env, dspec):
+        spack_cc    # Ensure spack module-scope variable is avaiable
+        check(from_cmake == "from_cmake",
+              "setup_dependent_environment couldn't read global set by cmake.")
+
+        check(self.spec['cmake'].link_arg == "test link arg",
+              "link arg on dependency spec not readable from setup_dependent_environment.")
+
+
+    def setup_dependent_package(self, module, dspec):
+        spack_cc    # Ensure spack module-scope variable is avaiable
+        check(from_cmake == "from_cmake",
+              "setup_dependent_package couldn't read global set by cmake.")
+
+        check(self.spec['cmake'].link_arg == "test link arg",
+              "link arg on dependency spec not readable from setup_dependent_package.")
+
+
+
+    def install(self, spec, prefix):
+        # check that cmake is in the global scope.
+        global cmake
+        check(cmake is not None, "No cmake was in environment!")
+
+        # check that which('cmake') returns the right one.
+        cmake = which('cmake')
+        check(cmake.exe[0].startswith(spec['cmake'].prefix.bin),
+              "Wrong cmake was in environment: %s" % cmake)
+
+        check(from_cmake == "from_cmake",
+              "Couldn't read global set by cmake.")
+
+        check(os.environ['from_cmake'] == 'from_cmake',
+              "Couldn't read env var set in envieonmnt by dependency")
+
+        mkdirp(prefix.bin)
+        touch(join_path(prefix.bin, 'dummy'))

var/spack/repos/builtin.mock/packages/cmake/package.py

@@ -0,0 +1,69 @@
+##############################################################################
+# Copyright (c) 2013, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Written by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/llnl/spack
+# Please also see the LICENSE file for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License (as published by
+# the Free Software Foundation) version 2.1 dated February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+import os
+
+def check(condition, msg):
+    """Raise an install error if condition is False."""
+    if not condition:
+        raise InstallError(msg)
+
+
+class Cmake(Package):
+    """A dumy package for the cmake build system."""
+    homepage  = 'https://www.cmake.org'
+    url       = 'https://cmake.org/files/v3.4/cmake-3.4.3.tar.gz'
+
+    version('3.4.3',    '4cb3ff35b2472aae70f542116d616e63',
+            url='https://cmake.org/files/v3.4/cmake-3.4.3.tar.gz')
+
+
+    def setup_environment(self, spack_env, run_env):
+        spack_cc    # Ensure spack module-scope variable is avaiable
+        spack_env.set('for_install', 'for_install')
+
+    def setup_dependent_environment(self, spack_env, run_env, dspec):
+        spack_cc    # Ensure spack module-scope variable is avaiable
+        spack_env.set('from_cmake', 'from_cmake')
+
+
+    def setup_dependent_package(self, module, dspec):
+        spack_cc    # Ensure spack module-scope variable is avaiable
+
+        self.spec.from_cmake = "from_cmake"
+        module.from_cmake = "from_cmake"
+
+        self.spec.link_arg = "test link arg"
+
+
+    def install(self, spec, prefix):
+        mkdirp(prefix.bin)
+
+        check(os.environ['for_install'] == 'for_install',
+              "Couldn't read env var set in compile envieonmnt")
+
+        cmake_exe = join_path(prefix.bin, 'cmake')
+        touch(cmake_exe)
+        set_executable(cmake_exe)