Merge pull request #6 in SCALE/spack from bugfix/SPACK-20-bin-directories-in-path to develop

# By Todd Gamblin
# Via Todd Gamblin
* commit '554ae9b3552a40ed253250bdebf548e4d8b01976':
  bugfix for SPACK-20: add dependency bin directories to PATH

4 files changed, 181 insertions, 126 deletions

diff --git a/lib/spack/env/cc b/lib/spack/env/cc
index 0711c873e3..e5dbf21beb 100755
--- a/lib/spack/env/cc
+++ b/lib/spack/env/cc
@@ -62,15 +62,17 @@ options, other_args = parser.parse_known_args()
 rpaths, other_args = parse_rpaths(other_args)
 
 # Add dependencies' include and lib paths to our compiler flags.
-def append_if_dir(path_list, *dirs):
-    full_path = os.path.join(*dirs)
-    if os.path.isdir(full_path):
-        path_list.append(full_path)
+def add_if_dir(path_list, directory, index=None):
+    if os.path.isdir(directory):
+        if index is None:
+            path_list.append(directory)
+        else:
+            path_list.insert(index, directory)
 
 for dep_dir in spack_deps:
-    append_if_dir(options.include_path, dep_dir, "include")
-    append_if_dir(options.lib_path,     dep_dir, "lib")
-    append_if_dir(options.lib_path,     dep_dir, "lib64")
+    add_if_dir(options.include_path, os.path.join(dep_dir, "include"))
+    add_if_dir(options.lib_path,     os.path.join(dep_dir, "lib"))
+    add_if_dir(options.lib_path,     os.path.join(dep_dir, "lib64"))
 
 # Add our modified arguments to it.
 arguments  = ['-I%s' % path for path in options.include_path]
@@ -95,11 +97,9 @@ for var in ["LD_LIBRARY_PATH", "LD_RUN_PATH", "DYLD_LIBRARY_PATH"]:
         os.environ.pop(var)
 
 # Ensure that the delegated command doesn't just call this script again.
-clean_path = get_path("PATH")
-for item in ['.'] + spack_env_path:
-    if item in clean_path:
-        clean_path.remove(item)
-os.environ["PATH"] = ":".join(clean_path)
+remove_paths = ['.'] + spack_env_path
+path = [p for p in get_path("PATH") if p not in remove_paths]
+os.environ["PATH"] = ":".join(path)
 
 full_command = [command] + arguments
 
diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
new file mode 100644
index 0000000000..d6becb77db
--- /dev/null
+++ b/lib/spack/spack/build_environment.py
@@ -0,0 +1,160 @@
+"""
+This module contains all routines related to setting up the package
+build environment.  All of this is set up by package.py just before
+install() is called.
+
+There are two parts to the bulid environment:
+
+1. Python build environment (i.e. install() method)
+
+   This is how things are set up when install() is called.  Spack
+   takes advantage of each package being in its own module by adding a
+   bunch of command-like functions (like configure(), make(), etc.) in
+   the package's module scope.  Ths allows package writers to call
+   them all directly in Package.install() without writing 'self.'
+   everywhere.  No, this isn't Pythonic.  Yes, it makes the code more
+   readable and more like the shell script from whcih someone is
+   likely porting.
+
+2. Build execution environment
+
+   This is the set of environment variables, like PATH, CC, CXX,
+   etc. that control the build.  There are also a number of
+   environment variables used to pass information (like RPATHs and
+   other information about dependencies) to Spack's compiler wrappers.
+   All of these env vars are also set up here.
+
+Skimming this module is a nice way to get acquainted with the types of
+calls you can make from within the install() function.
+"""
+import os
+import shutil
+import multiprocessing
+import platform
+from llnl.util.filesystem import *
+
+import spack
+from spack.util.executable import Executable, which
+from spack.util.environment import *
+
+#
+# This can be set by the user to globally disable parallel builds.
+#
+SPACK_NO_PARALLEL_MAKE = 'SPACK_NO_PARALLEL_MAKE'
+
+#
+# These environment variables are set by
+# set_build_environment_variables and used to pass parameters to
+# Spack's compiler wrappers.
+#
+SPACK_LIB              = 'SPACK_LIB'
+SPACK_ENV_PATH         = 'SPACK_ENV_PATH'
+SPACK_DEPENDENCIES     = 'SPACK_DEPENDENCIES'
+SPACK_PREFIX           = 'SPACK_PREFIX'
+SPACK_BUILD_ROOT       = 'SPACK_BUILD_ROOT'
+
+
+class MakeExecutable(Executable):
+    """Special callable executable object for make so the user can
+       specify parallel or not on a per-invocation basis.  Using
+       'parallel' as a kwarg will override whatever the package's
+       global setting is, so you can either default to true or false
+       and override particular calls.
+
+       Note that if the SPACK_NO_PARALLEL_MAKE env var is set it overrides
+       everything.
+    """
+    def __init__(self, name, parallel):
+        super(MakeExecutable, self).__init__(name)
+        self.parallel = parallel
+
+    def __call__(self, *args, **kwargs):
+        parallel = kwargs.get('parallel', self.parallel)
+        disable_parallel = env_flag(SPACK_NO_PARALLEL_MAKE)
+
+        if parallel and not disable_parallel:
+            jobs = "-j%d" % multiprocessing.cpu_count()
+            args = (jobs,) + args
+
+        super(MakeExecutable, self).__call__(*args, **kwargs)
+
+
+def set_build_environment_variables(pkg):
+    """This ensures a clean install environment when we build packages.
+    """
+    # This tells the compiler script where to find the Spack installation.
+    os.environ[SPACK_LIB] = spack.lib_path
+
+    # Add spack build environment path with compiler wrappers first in
+    # the path.  We handle case sensitivity conflicts like "CC" and
+    # "cc" by putting one in the <build_env_path>/case-insensitive
+    # directory.  Add that to the path too.
+    env_paths = [spack.build_env_path,
+                 join_path(spack.build_env_path, 'case-insensitive')]
+    path_put_first("PATH", env_paths)
+    path_set(SPACK_ENV_PATH, env_paths)
+
+    # Prefixes of all of the package's dependencies go in
+    # SPACK_DEPENDENCIES
+    dep_prefixes = [d.package.prefix for d in pkg.spec.dependencies.values()]
+    path_set(SPACK_DEPENDENCIES, dep_prefixes)
+
+    # Install prefix
+    os.environ[SPACK_PREFIX] = pkg.prefix
+
+    # Build root for logging.
+    os.environ[SPACK_BUILD_ROOT] = pkg.stage.expanded_archive_path
+
+    # Remove these vars from the environment during build becaus they
+    # can affect how some packages find libraries.  We want to make
+    # sure that builds never pull in unintended external dependencies.
+    pop_keys(os.environ, "LD_LIBRARY_PATH", "LD_RUN_PATH", "DYLD_LIBRARY_PATH")
+
+    # Add bin directories from dependencies to the PATH for the build.
+    bin_dirs = ['%s/bin' % prefix for prefix in dep_prefixes]
+    path_put_first('PATH', [bin for bin in bin_dirs if os.path.isdir(bin)])
+
+
+def set_module_variables_for_package(pkg):
+    """Populate the module scope of install() with some useful functions.
+       This makes things easier for package writers.
+    """
+    m = pkg.module
+
+    m.make  = MakeExecutable('make', pkg.parallel)
+    m.gmake = MakeExecutable('gmake', pkg.parallel)
+
+    # number of jobs spack prefers to build with.
+    m.make_jobs = multiprocessing.cpu_count()
+
+    # Find the configure script in the archive path
+    # Don't use which for this; we want to find it in the current dir.
+    m.configure = Executable('./configure')
+
+    # TODO: shouldn't really use "which" here.  Consider adding notion
+    # 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")
+
+    # standard CMake arguments
+    m.std_cmake_args = ['-DCMAKE_INSTALL_PREFIX=%s' % pkg.prefix,
+                        '-DCMAKE_BUILD_TYPE=None']
+    if platform.mac_ver()[0]:
+        m.std_cmake_args.append('-DCMAKE_FIND_FRAMEWORK=LAST')
+
+    # Emulate some shell commands for convenience
+    m.cd         = os.chdir
+    m.mkdir      = os.mkdir
+    m.makedirs   = os.makedirs
+    m.remove     = os.remove
+    m.removedirs = os.removedirs
+
+    m.mkdirp     = mkdirp
+    m.install    = install
+    m.rmtree     = shutil.rmtree
+    m.move       = shutil.move
+
+    # Useful directories within the prefix are encapsulated in
+    # a Prefix object.
+    m.prefix  = pkg.prefix
diff --git a/lib/spack/spack/globals.py b/lib/spack/spack/globals.py
index 4ff50a3e7e..096c1334f5 100644
--- a/lib/spack/spack/globals.py
+++ b/lib/spack/spack/globals.py
@@ -40,7 +40,7 @@ spack_file = join_path(prefix, "bin", "spack")
 
 # spack directory hierarchy
 lib_path       = join_path(prefix, "lib", "spack")
-env_path       = join_path(lib_path, "env")
+build_env_path = join_path(lib_path, "env")
 module_path    = join_path(lib_path, "spack")
 compilers_path = join_path(module_path, "compilers")
 test_path      = join_path(module_path, "test")
@@ -130,10 +130,3 @@ sys_type = None
 #
 mirrors = []
 
-# Important environment variables
-SPACK_NO_PARALLEL_MAKE = 'SPACK_NO_PARALLEL_MAKE'
-SPACK_LIB = 'SPACK_LIB'
-SPACK_ENV_PATH = 'SPACK_ENV_PATH'
-SPACK_DEPENDENCIES = 'SPACK_DEPENDENCIES'
-SPACK_PREFIX = 'SPACK_PREFIX'
-SPACK_BUILD_ROOT = 'SPACK_BUILD_ROOT'
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 27b7b8aef5..b8f66bbb76 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -38,25 +38,22 @@ import os
 import re
 import subprocess
 import platform as py_platform
-import shutil
 import multiprocessing
 from urlparse import urlparse
 
 import llnl.util.tty as tty
-from llnl.util.tty.color import cwrite
 from llnl.util.filesystem import *
 from llnl.util.lang import *
 
 import spack
 import spack.spec
 import spack.error
+import spack.build_environment as build_env
 import spack.url as url
 import spack.util.crypto as crypto
 from spack.version import *
 from spack.stage import Stage
 from spack.util.web import get_pages
-from spack.util.environment import *
-from spack.util.executable import Executable, which
 from spack.util.compression import allowed_archive
 
 """Allowed URL schemes for spack packages."""
@@ -413,46 +410,6 @@ class Package(object):
         return self._stage
 
 
-    def add_commands_to_module(self):
-        """Populate the module scope of install() with some useful functions.
-           This makes things easier for package writers.
-        """
-        m = self.module
-
-        m.make  = MakeExecutable('make', self.parallel)
-        m.gmake = MakeExecutable('gmake', self.parallel)
-
-        # number of jobs spack prefers to build with.
-        m.make_jobs = multiprocessing.cpu_count()
-
-        # Find the configure script in the archive path
-        # Don't use which for this; we want to find it in the current dir.
-        m.configure = Executable('./configure')
-        m.cmake = which("cmake")
-
-        # standard CMake arguments
-        m.std_cmake_args = ['-DCMAKE_INSTALL_PREFIX=%s' % self.prefix,
-                            '-DCMAKE_BUILD_TYPE=None']
-        if py_platform.mac_ver()[0]:
-            m.std_cmake_args.append('-DCMAKE_FIND_FRAMEWORK=LAST')
-
-        # Emulate some shell commands for convenience
-        m.cd         = os.chdir
-        m.mkdir      = os.mkdir
-        m.makedirs   = os.makedirs
-        m.remove     = os.remove
-        m.removedirs = os.removedirs
-
-        m.mkdirp     = mkdirp
-        m.install    = install
-        m.rmtree     = shutil.rmtree
-        m.move       = shutil.move
-
-        # Useful directories within the prefix are encapsulated in
-        # a Prefix object.
-        m.prefix  = self.prefix
-
-
     def preorder_traversal(self, visited=None, **kwargs):
         """This does a preorder traversal of the package's dependence DAG."""
         virtual = kwargs.get("virtual", False)
@@ -682,19 +639,16 @@ class Package(object):
             self.do_install_dependencies()
 
         self.do_patch()
-        self.setup_install_environment()
-
-        # Add convenience commands to the package's module scope to
-        # make building easier.
-        self.add_commands_to_module()
-
-        tty.msg("Building %s." % self.name)
 
         # create the install directory (allow the layout to handle this in
         # case it needs to add extra files)
         spack.install_layout.make_path_for_spec(self.spec)
 
+        tty.msg("Building %s." % self.name)
         try:
+            build_env.set_build_environment_variables(self)
+            build_env.set_module_variables_for_package(self)
+
             self.install(self.spec, self.prefix)
             if not os.path.isdir(self.prefix):
                 tty.die("Install failed for %s.  No install dir created." % self.name)
@@ -713,36 +667,6 @@ class Package(object):
                 self.stage.destroy()
 
 
-    def setup_install_environment(self):
-        """This ensures a clean install environment when we build packages."""
-        pop_keys(os.environ, "LD_LIBRARY_PATH", "LD_RUN_PATH", "DYLD_LIBRARY_PATH")
-
-        # Add spack environment at front of path and pass the
-        # lib location along so the compiler script can find spack
-        os.environ[spack.SPACK_LIB] = spack.lib_path
-
-        # Fix for case-insensitive file systems.  Conflicting links are
-        # in directories called "case*" within the env directory.
-        env_paths = [spack.env_path]
-        for file in os.listdir(spack.env_path):
-            path = join_path(spack.env_path, file)
-            if file.startswith("case") and os.path.isdir(path):
-                env_paths.append(path)
-        path_put_first("PATH", env_paths)
-        path_set(spack.SPACK_ENV_PATH, env_paths)
-
-        # Pass along prefixes of dependencies here
-        path_set(
-            spack.SPACK_DEPENDENCIES,
-            [dep.package.prefix for dep in self.spec.dependencies.values()])
-
-        # Install location
-        os.environ[spack.SPACK_PREFIX] = self.prefix
-
-        # Build root for logging.
-        os.environ[spack.SPACK_BUILD_ROOT] = self.stage.expanded_archive_path
-
-
     def do_install_dependencies(self):
         # Pass along paths of dependencies here
         for dep in self.spec.dependencies.values():
@@ -786,7 +710,8 @@ class Package(object):
     def clean(self):
         """By default just runs make clean.  Override if this isn't good."""
         try:
-            make = MakeExecutable('make', self.parallel)
+            # TODO: should we really call make clean, ro just blow away the directory?
+            make = build_env.MakeExecutable('make', self.parallel)
             make('clean')
             tty.msg("Successfully cleaned %s" % self.name)
         except subprocess.CalledProcessError, e:
@@ -871,30 +796,6 @@ def find_versions_of_archive(archive_url, **kwargs):
     return versions
 
 
-class MakeExecutable(Executable):
-    """Special Executable for make so the user can specify parallel or
-       not on a per-invocation basis.  Using 'parallel' as a kwarg will
-       override whatever the package's global setting is, so you can
-       either default to true or false and override particular calls.
-
-       Note that if the SPACK_NO_PARALLEL_MAKE env var is set it overrides
-       everything.
-    """
-    def __init__(self, name, parallel):
-        super(MakeExecutable, self).__init__(name)
-        self.parallel = parallel
-
-    def __call__(self, *args, **kwargs):
-        parallel = kwargs.get('parallel', self.parallel)
-        disable_parallel = env_flag(spack.SPACK_NO_PARALLEL_MAKE)
-
-        if parallel and not disable_parallel:
-            jobs = "-j%d" % multiprocessing.cpu_count()
-            args = (jobs,) + args
-
-        super(MakeExecutable, self).__call__(*args, **kwargs)
-
-
 def validate_package_url(url_string):
     """Determine whether spack can handle a particular URL or not."""
     url = urlparse(url_string)
@@ -911,6 +812,7 @@ def print_pkg(message):
     if mac_ver and Version(mac_ver) >= Version('10.7'):
         print u"\U0001F4E6" + tty.indent,
     else:
+        from llnl.util.tty.color import cwrite
         cwrite('@*g{[+]} ')
     print message