Extract installation of dependencies from PackageBase.do_install

alalazo · alalazo · commit 48ef453694ef · 2018-01-05T08:43:41.000+01:00
Up to this commit PackageBase.do_install was installing the root package
of a DAG unconditionally and its dependencies conditionally (depending
on the value of the boolean argument 'install_deps').

However, a sensible operation in many contexts is to install *only* the
dependencies. This was not possible via API, and the code to do that
was replicated in the implementation of PackageBase.do_install and of
the 'spack install' command. Further, the code in those two places was
not doing *exactly* the same thing.

This commit extracts a new function that is used in both places, and is
external to PackageBase. This choice has been preferred over extending
PackageBase.do_install API because the method was already complex enough
(10-15 arguments) and needed to be simplified + having a method on an
instance that installs anything but that instance spec didn't seem a
sensible choice.
diff --git a/lib/spack/spack/cmd/bootstrap.py b/lib/spack/spack/cmd/bootstrap.py
@@ -26,6 +26,7 @@
 import spack
 import spack.cmd
 import spack.cmd.common.arguments as arguments
+import spack.package
 
 description = "Bootstrap packages needed for spack to run smoothly"
 section = "admin"
@@ -86,5 +87,4 @@ def bootstrap(parser, args, **kwargs):
             tty.msg("Installing %s to satisfy requirement for %s" %
                     (spec_to_install, requirement))
             kwargs['explicit'] = True
-            package = spack.repo.get(spec_to_install)
-            package.do_install(**kwargs)
+            spack.package.install(spec_to_install, **kwargs)
diff --git a/lib/spack/spack/cmd/configure.py b/lib/spack/spack/cmd/configure.py
@@ -86,7 +86,7 @@ def _stop_at_phase_during_install(args, calling_fn, phase_mapping):
         inst.install(parser, install_args)
         # Install package and stop at the given phase
         cli_args = ['-v'] if args.verbose else []
-        install_args = parser.parse_args(cli_args + ['--only=package'])
+        install_args = parser.parse_args(cli_args + ['--only=root'])
         install_args.package = args.package
         inst.install(parser, install_args, stop_at=phase)
     except IndexError:
diff --git a/lib/spack/spack/cmd/diy.py b/lib/spack/spack/cmd/diy.py
@@ -22,7 +22,6 @@
 # License along with this program; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 ##############################################################################
-import sys
 import os
 import argparse
 
@@ -78,22 +77,23 @@ def diy(self, args):
             "Did you forget a package version number?")
 
     spec.concretize()
-    package = spack.repo.get(spec)
 
-    if package.installed:
-        tty.error("Already installed in %s" % package.prefix)
-        tty.msg("Uninstall or try adding a version suffix for this DIY build.")
-        sys.exit(1)
+    if spec.package.installed:
+        msg = "Already installed in {0}. "
+        msg += "Uninstall or try adding a version suffix for this DIY build."
+        tty.die(msg.format(spec.prefix))
 
     # Forces the build to run out of the current directory.
-    package.stage = DIYStage(os.getcwd())
+    spec.package.stage = DIYStage(os.getcwd())
 
     # TODO: make this an argument, not a global.
     spack.do_checksum = False
 
-    package.do_install(
+    spack.package.install(
+        spec,
         keep_prefix=args.keep_prefix,
         install_deps=not args.ignore_deps,
         verbose=not args.quiet,
         keep_stage=True,   # don't remove source dir for DIY.
-        dirty=args.dirty)
+        dirty=args.dirty
+    )
diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
@@ -50,9 +50,9 @@
 def setup_parser(subparser):
     subparser.add_argument(
         '--only',
-        default='package,dependencies',
+        default='root,dependencies',
         dest='things_to_install',
-        choices=['package', 'dependencies'],
+        choices=['root', 'dependencies'],
         help="""select the mode of installation.
 the default is to install the package along with all its dependencies.
 alternatively one can decide to install only the package or only
@@ -344,19 +344,8 @@ def install_spec(cli_args, kwargs, spec):
     try:
         # decorate the install if necessary
         PackageBase.do_install = decorator(PackageBase.do_install)
-
-        if cli_args.things_to_install == 'dependencies':
-            # Install dependencies as-if they were installed
-            # for root (explicit=False in the DB)
-            kwargs['explicit'] = False
-            for s in spec.dependencies():
-                p = spack.repo.get(s)
-                p.do_install(**kwargs)
-        else:
-            package = spack.repo.get(spec)
-            kwargs['explicit'] = True
-            package.do_install(**kwargs)
-
+        what = cli_args.things_to_install.split(',')
+        spack.package.install(spec, what=what, **kwargs)
     except InstallError as e:
         if cli_args.show_log_on_error:
             e.print_context()
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
@@ -48,6 +48,7 @@
 from six import with_metaclass
 
 import llnl.util.tty as tty
+import llnl.util.filesystem as filesystem
 import spack
 import spack.store
 import spack.compilers
@@ -63,7 +64,7 @@
 import spack.binary_distribution as binary_distribution
 
 from llnl.util.filesystem import mkdirp, join_path, touch, ancestor
-from llnl.util.filesystem import working_dir, install_tree, install
+from llnl.util.filesystem import working_dir, install_tree
 from llnl.util.lang import memoized
 from llnl.util.link_tree import LinkTree
 from llnl.util.tty.log import log_output
@@ -552,6 +553,9 @@ class SomePackage(Package):
     #: index of patches by sha256 sum, built lazily
     _patches_by_hash = None
 
+    #: Used to stop early during installation, if set in instances
+    last_phase = None
+
     #: List of strings which contains GitHub usernames of package maintainers.
     #: Do not include @ here in order not to unnecessarily ping the users.
     maintainers = []
@@ -1287,18 +1291,18 @@ def do_install(self,
                    keep_prefix=False,
                    keep_stage=False,
                    install_source=False,
-                   install_deps=True,
                    skip_patch=False,
                    verbose=False,
                    make_jobs=None,
                    fake=False,
                    explicit=False,
                    dirty=None,
                    **kwargs):
-        """Called by commands to install a package and its dependencies.
+        """Called by :py:func:`install` to install *this* package.
 
-        Package implementations should override install() to describe
-        their build process.
+        This method installs only ``self.spec`` and assumes all its
+        dependencies are already installed and the spec itself is
+        concrete.
 
         Args:
             keep_prefix (bool): Keep install prefix on failure. By default,
@@ -1308,8 +1312,6 @@ def do_install(self,
                 even with exceptions.
             install_source (bool): By default, source is not installed, but
                 for debugging it might be useful to keep it around.
-            install_deps (bool): Install dependencies before installing this
-                package
             skip_patch (bool): Skip patch stage of build if True.
             verbose (bool): Display verbose build output (by default,
                 suppresses it)
@@ -1319,11 +1321,13 @@ def do_install(self,
             explicit (bool): True if package was explicitly installed, False
                 if package was implicitly installed (as a dependency).
             dirty (bool): Don't clean the build environment before installing.
-            force (bool): Install again, even if already installed.
+
+        Raises:
+            ValueError: if the ``self.spec`` is not concrete
         """
         if not self.spec.concrete:
-            raise ValueError("Can only install concrete packages: %s."
-                             % self.spec.name)
+            msg = "Can only install concrete packages: {0}."
+            raise ValueError(msg.format(self.spec.name))
 
         # For external packages the workflow is simplified, and basically
         # consists in module file generation and registration in the DB
@@ -1351,25 +1355,6 @@ def do_install(self,
 
                 return self._update_explicit_entry_in_db(rec, explicit)
 
-        self._do_install_pop_kwargs(kwargs)
-
-        # First, install dependencies recursively.
-        if install_deps:
-            tty.debug('Installing {0} dependencies'.format(self.name))
-            for dep in self.spec.traverse(order='post', root=False):
-                dep.package.do_install(
-                    install_deps=False,
-                    explicit=False,
-                    keep_prefix=keep_prefix,
-                    keep_stage=keep_stage,
-                    install_source=install_source,
-                    fake=fake,
-                    skip_patch=skip_patch,
-                    verbose=verbose,
-                    make_jobs=make_jobs,
-                    dirty=dirty,
-                    **kwargs)
-
         tty.msg(colorize('@*{Installing} @*g{%s}' % self.name))
 
         if kwargs.get('use_cache', False):
@@ -1542,19 +1527,6 @@ def check_for_unfinished_installation(
 
         return partial
 
-    def _do_install_pop_kwargs(self, kwargs):
-        """Pops kwargs from do_install before starting the installation
-
-        Args:
-            kwargs:
-              'stop_at': last installation phase to be executed (or None)
-
-        """
-        self.last_phase = kwargs.pop('stop_at', None)
-        if self.last_phase is not None and self.last_phase not in self.phases:
-            tty.die('\'{0}\' is not an allowed phase for package {1}'
-                    .format(self.last_phase, self.name))
-
     def log(self):
         # Copy provenance into the install directory on success
         log_install_path = spack.store.layout.build_log_path(self.spec)
@@ -1570,8 +1542,8 @@ def log(self):
             # FIXME : this potentially catches too many things...
             pass
 
-        install(self.log_path, log_install_path)
-        install(self.env_path, env_install_path)
+        filesystem.install(self.log_path, log_install_path)
+        filesystem.install(self.env_path, env_install_path)
         dump_packages(self.spec, packages_dir)
 
     def sanity_check_prefix(self):
@@ -2083,6 +2055,51 @@ class Package(PackageBase):
     run_after('install')(PackageBase.sanity_check_prefix)
 
 
+def install(spec, what=('root', 'dependencies'), stop_at=None, **kwargs):
+    """Install a concretized spec and / or its dependencies.
+
+    This function permits to install either the entire DAG, only the
+    dependencies or only the root package. In any case the root package
+    will be installed as 'explicit', while the children as 'non-explicit'.
+
+    The keyword arguments are forwarded directly to
+    :py:meth:`PackageBase.do_install` during the installation of the
+    various nodes.
+
+    Args:
+        spec (Spec): spec to be installed. Must be concrete.
+        what (tuple of str): specifies what needs to be installed. If 'root'
+            is in the argument, then the root package of the spec will be
+            installed. If 'dependencies' is in the argument the child nodes
+            of the DAG will be installed. By default it installs both.
+        stop_at (str or None): if specified, stops the installation at the
+            given phase. Valid only for the root package.
+        **kwargs: keyword arguments to be forwarded to
+            :py:meth:`PackageBase.do_install`.
+    """
+    # Check if we should stop early during the installation of the root spec
+    pkg = spec.package
+    pkg.last_phase = stop_at
+
+    if pkg.last_phase is not None and pkg.last_phase not in pkg.phases:
+        msg = '"{0}" is not an allowed phase for package {1}'
+        raise ValueError(msg.format(pkg.last_phase, pkg.name))
+
+    # Remove the keyword explicit if present: the root spec will be marked
+    # explicit, all the child specs will be non-explicit
+    kwargs.pop('explicit', None)
+
+    # Install the dependencies if required to, traversing the DAG
+    # in post order and omitting root
+    if 'dependencies' in what:
+        tty.debug('Installing {0} dependencies'.format(spec.name))
+        for dep in spec.traverse(order='post', root=False):
+            dep.package.do_install(explicit=False, **kwargs)
+
+    if 'root' in what:
+        spec.package.do_install(explicit=True, **kwargs)
+
+
 def install_dependency_symlinks(pkg, spec, prefix):
     """Execute a dummy install and flatten dependencies"""
     flatten_dependencies(spec, prefix)
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
@@ -28,6 +28,7 @@
 import re
 import spack
 import spack.cmd
+import spack.package
 from spack.util.executable import Executable, ProcessError
 from llnl.util.filesystem import filter_file
 import llnl.util.tty as tty
@@ -42,10 +43,9 @@ def get_patchelf():
     # as we may need patchelf, find out where it is
     if platform.system() == 'Darwin':
         return None
-    patchelf_spec = spack.cmd.parse_specs("patchelf", concretize=True)[0]
-    patchelf = spack.repo.get(patchelf_spec)
+    patchelf = spack.cmd.parse_specs("patchelf", concretize=True)[0]
     if not patchelf.installed:
-        patchelf.do_install()
+        spack.package.install(patchelf)
     patchelf_executable = os.path.join(patchelf.prefix.bin, "patchelf")
     return patchelf_executable
 
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
@@ -32,6 +32,7 @@
 
 import spack
 import spack.cmd.install
+import spack.package
 from spack.spec import Spec
 from spack.main import SpackCommand, SpackCommandError
 
@@ -139,8 +140,7 @@ def test_package_output(tmpdir, capsys, install_mockery, mock_fetch):
     # logging. TODO: see whether we can get multiple log_outputs to work
     # when nested AND in pytest
     spec = Spec('printing-package').concretized()
-    pkg = spec.package
-    pkg.do_install(verbose=True)
+    spack.package.install(spec, verbose=True)
 
     log_file = os.path.join(spec.prefix, '.spack', 'build.out')
     with open(log_file) as f:
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
@@ -40,12 +40,12 @@
 import spack.database
 import spack.directory_layout
 import spack.platforms.test
+import spack.package
 import spack.repository
 import spack.stage
 import spack.util.executable
 import spack.util.pattern
 from spack.dependency import Dependency
-from spack.package import PackageBase
 from spack.fetch_strategy import FetchStrategyComposite, URLFetchStrategy
 from spack.fetch_strategy import FetchError
 from spack.spec import Spec
@@ -306,8 +306,7 @@ def database(tmpdir_factory, builtin_mock, config):
     def _install(spec):
         s = spack.spec.Spec(spec)
         s.concretize()
-        pkg = spack.repo.get(s)
-        pkg.do_install(fake=True)
+        spack.package.install(s, fake=True)
 
     def _uninstall(spec):
         spec.package.do_uninstall(spec)
@@ -393,10 +392,10 @@ def mock_fetch(mock_archive):
     def fake_fn(self):
         return fetcher
 
-    orig_fn = PackageBase.fetcher
-    PackageBase.fetcher = fake_fn
+    orig_fn = spack.package.PackageBase.fetcher
+    spack.package.PackageBase.fetcher = fake_fn
     yield
-    PackageBase.fetcher = orig_fn
+    spack.package.PackageBase.fetcher = orig_fn
 
 
 ##########
diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py
@@ -33,6 +33,7 @@
 from llnl.util.tty.colify import colify
 
 import spack
+import spack.package
 import spack.store
 from spack.test.conftest import MockPackageMultiRepo
 from spack.util.executable import Executable
@@ -108,8 +109,7 @@ def _check_db_sanity(install_db):
 def _mock_install(spec):
     s = spack.spec.Spec(spec)
     s.concretize()
-    pkg = spack.repo.get(s)
-    pkg.do_install(fake=True)
+    spack.package.install(s, fake=True)
 
 
 def _mock_remove(spec):
@@ -417,7 +417,7 @@ def test_external_entries_in_db(database):
     assert rec.spec.external_module is None
     assert rec.explicit is False
 
-    rec.spec.package.do_install(fake=True, explicit=True)
+    spack.package.install(rec.spec, fake=True)
     rec = install_db.get_record('externaltool')
     assert rec.spec.external_path == '/path/to/external_tool'
     assert rec.spec.external_module is None
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
