spack · tgamblin · May 7, 2021 · Apr 29, 2021 · Apr 30, 2021 · Apr 30, 2021
diff --git a/lib/spack/docs/environments.rst b/lib/spack/docs/environments.rst
@@ -356,6 +356,18 @@ command also stores a Spack repo containing the ``package.py`` file
 used at install time for each package in the ``repos/`` directory in
 the Environment.
 
+The ``--no-add`` option can be used in a concrete environment to tell
+spack to install specs already present in the environment but not to
+add any new root specs to the environment.  For root specs provided
+to ``spack install`` on the command line, ``--no-add`` is the default,
+while for dependency specs on the other hand, it is optional.  In other
+words, if there is an unambiguous match in the active concrete environment
+for a root spec provided to ``spack install`` on the command line, spack
+does not require you to specify the ``--no-add` option to prevent the spec
+from being added again.  At the same time, a spec that already exists in the
+environment, but only as a dependency, will be added to the environment as a
+root spec without the ``--no-add`` option.
+
 ^^^^^^^
 Loading
 ^^^^^^^

diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
@@ -139,6 +139,10 @@ def setup_parser(subparser):
     subparser.add_argument(
         '--only-concrete', action='store_true', default=False,
         help='(with environment) only install already concretized specs')
+    subparser.add_argument(
+        '--no-add', action='store_true', default=False,
+        help="""(with environment) only install specs provided as argument
+if they are already in the concretized environment""")
     subparser.add_argument(
         '-f', '--file', action='append', default=[],
         dest='specfiles', metavar='SPEC_YAML_FILE',
@@ -205,11 +209,66 @@ def install_specs(cli_args, kwargs, specs):
 
     try:
         if env:
+            specs_to_install = []
+            specs_to_add = []
             for abstract, concrete in specs:
-                with env.write_transaction():
-                    concrete = env.concretize_and_add(abstract, concrete)
-                    env.write(regenerate_views=False)
-            env.install_all(cli_args, **kwargs)
+                # This won't find specs added to the env since last
+                # concretize, therefore should we consider enforcing
+                # concretization of the env before allowing to install
+                # specs?
+                m_spec = env.matching_spec(abstract)
+
+                # If there is any ambiguity in the above call to matching_spec
+                # (i.e. if more than one spec in the environment matches), then
+                # SpackEnvironmentError is rasied, with a message listing the
+                # the matches.  Getting to this point means there were either
+                # no matches or exactly one match.
+
+                if not m_spec:
+                    tty.debug('{0} matched nothing in the env'.format(
+                        abstract.name))
+                    # no matches in the env
+                    if cli_args.no_add:
+                        msg = ('You asked to install {0} without adding it ' +
+                               '(--no-add), but no such spec exists in ' +
+                               'environment').format(abstract.name)
+                        tty.die(msg)
+                    else:
+                        tty.debug('adding {0} as a root'.format(abstract.name))
+                        specs_to_add.append((abstract, concrete))
+
+                    continue
+
+                tty.debug('exactly one match for {0} in env -> {1}'.format(
+                    m_spec.name, m_spec.dag_hash()))
+
+                if m_spec in env.roots() or cli_args.no_add:
+                    # either the single match is a root spec (and --no-add is
+                    # the default for roots) or --no-add was stated explictly
+                    tty.debug('just install {0}'.format(m_spec.name))
+                    specs_to_install.append(m_spec)
+                else:
+                    # the single match is not a root (i.e. it's a dependency),
+                    # and --no-add was not specified, so we'll add it as a
+                    # root before installing
+                    tty.debug('add {0} then install it'.format(m_spec.name))
+                    specs_to_add.append((abstract, concrete))
+
+            if specs_to_add:
+                tty.debug('Adding the following specs as roots:')
+                for abstract, concrete in specs_to_add:
+                    tty.debug('  {0}'.format(abstract.name))
+                    with env.write_transaction():
+                        specs_to_install.append(
+                            env.concretize_and_add(abstract, concrete))
+                        env.write(regenerate_views=False)
+
+            # Install the validated list of cli specs
+            if specs_to_install:
+                tty.debug('Installing the following cli specs:')
+                for s in specs_to_install:
+                    tty.debug('  {0}'.format(s.name))
+                env.install_specs(specs_to_install, args=cli_args, **kwargs)
         else:
             installs = [(concrete.package, kwargs) for _, concrete in specs]
             builder = PackageInstaller(installs)

diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
@@ -1440,21 +1440,31 @@ def install_all(self, args=None, **install_args):
             args (Namespace): argparse namespace with command arguments
             install_args (dict): keyword install arguments
         """
+        self.install_specs(None, args=args, **install_args)
+
+    def install_specs(self, specs=None, args=None, **install_args):
         from spack.installer import PackageInstaller
+
         tty.debug('Assessing installation status of environment packages')
         # If "spack install" is invoked repeatedly for a large environment
         # where all specs are already installed, the operation can take
         # a large amount of time due to repeatedly acquiring and releasing
         # locks, this does an initial check across all specs within a single
-        # DB read transaction to reduce time spent in this case.
-        specs_to_install = self.uninstalled_specs()
+        # DB read transaction to reduce time spent in this case. In the next
+        # three lines we remove any already-installed root specs from the list
+        # to install.  However, uninstalled_specs() only considers root specs,
+        # so this will allow dep specs to be unnecessarily re-installed.
+        uninstalled_roots = self.uninstalled_specs()
+        specs_to_install = specs or uninstalled_roots
+        specs_to_install = [s for s in specs_to_install
+                            if s not in self.roots() or s in uninstalled_roots]
 
         if not specs_to_install:
             tty.msg('All of the packages are already installed')
             return
 
-        tty.debug('Processing {0} uninstalled specs'
-                  .format(len(specs_to_install)))
+        tty.debug('Processing {0} uninstalled specs'.format(
+            len(specs_to_install)))
 
         install_args['overwrite'] = install_args.get(
             'overwrite', []) + self._get_overwrite_specs()
@@ -1574,7 +1584,7 @@ def matching_spec(self, spec):
                             if abstract)
 
         if len(root_matches) == 1:
-            return root_matches[0][1]
+            return list(root_matches.items())[0][0]
 
         # More than one spec matched, and either multiple roots matched or
         # none of the matches were roots

diff --git a/lib/spack/spack/test/cmd/common/arguments.py b/lib/spack/spack/test/cmd/common/arguments.py
@@ -96,3 +96,21 @@ def test_multiple_env_match_raises_error(mock_packages, mutable_mock_env_path):
             spack.cmd.matching_spec_from_env(spack.cmd.parse_specs(['a'])[0])
 
     assert 'matches multiple specs' in exc_info.value.message
+
+
+@pytest.mark.usefixtures('config')
+def test_root_and_dep_match_returns_root(mock_packages, mutable_mock_env_path):
+    e = ev.create('test')
+    e.add('[email protected]')
+    e.add('a foobar=bar')  # Depends on b, should choose [email protected]
+    e.concretize()
+    with e:
+        # This query matches the root b and b as a dependency of a. In that
+        # case the root instance should be preferred.
+        env_spec1 = spack.cmd.matching_spec_from_env(
+            spack.cmd.parse_specs(['b'])[0])
+        assert env_spec1.satisfies('@0.9')
+
+        env_spec2 = spack.cmd.matching_spec_from_env(
+            spack.cmd.parse_specs(['[email protected]'])[0])
+        assert env_spec2
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
@@ -32,6 +32,7 @@
 mirror = SpackCommand('mirror')
 uninstall = SpackCommand('uninstall')
 buildcache = SpackCommand('buildcache')
+find = SpackCommand('find')
 
 
 @pytest.fixture()
@@ -712,6 +713,112 @@ def test_install_only_dependencies_of_all_in_env(
                 assert os.path.exists(dep.prefix)
 
 
+def test_install_no_add_in_env(tmpdir, mock_fetch, install_mockery,
+                               mutable_mock_env_path):
+    # To test behavior of --no-add option, we create the following environment:
+    #
+    #     mpileaks
+    #         ^callpath
+    #             ^dyninst
+    #                 ^[email protected]     # or latest, really
+    #                 ^libdwarf
+    #         ^mpich
+    #     [email protected]
+    #     a~bvv
+    #         ^b
+    #     a
+    #         ^b
+    e = ev.create('test')
+    e.add('mpileaks')
+    e.add('[email protected]')  # so env has both root and dep libelf specs
+    e.add('a')
+    e.add('a ~bvv')
+    e.concretize()
+    env_specs = e.all_specs()
+
+    a_spec = None
+    b_spec = None
+    mpi_spec = None
+
+    # First find and remember some target concrete specs in the environment
+    for e_spec in env_specs:
+        if e_spec.satisfies(Spec('a ~bvv')):
+            a_spec = e_spec
+        elif e_spec.name == 'b':
+            b_spec = e_spec
+        elif e_spec.satisfies(Spec('mpi')):
+            mpi_spec = e_spec
+
+    assert(a_spec)
+    assert(a_spec.concrete)
+
+    assert(b_spec)
+    assert(b_spec.concrete)
+    assert(b_spec not in e.roots())
+
+    assert(mpi_spec)
+    assert(mpi_spec.concrete)
+
+    # Activate the environment
+    with e:
+        # Assert using --no-add with a spec not in the env fails
+        inst_out = install(
+            '--no-add', 'boost', fail_on_error=False, output=str)
+
+        assert('no such spec exists in environment' in inst_out)
+
+        # Ensure using --no-add with an ambiguous spec fails
+        with pytest.raises(ev.SpackEnvironmentError) as err:
+            inst_out = install(
+                '--no-add', 'a', output=str)
+
+        assert('a matches multiple specs in the env' in str(err))
+
+        # With "--no-add", install an unambiguous dependency spec (that already
+        # exists as a dep in the environment) using --no-add and make sure it
+        # gets installed (w/ deps), but is not added to the environment.
+        install('--no-add', 'dyninst')
+
+        find_output = find('-l', output=str)
+        assert('dyninst' in find_output)
+        assert('libdwarf' in find_output)
+        assert('libelf' in find_output)
+        assert('callpath' not in find_output)
+
+        post_install_specs = e.all_specs()
+        assert all([s in env_specs for s in post_install_specs])
+
+        # Make sure we can install a concrete dependency spec from a spec.yaml
+        # file on disk, using the ``--no-add` option, and the spec is installed
+        # but not added as a root
+        mpi_spec_yaml_path = tmpdir.join('{0}.yaml'.format(mpi_spec.name))
+        with open(mpi_spec_yaml_path.strpath, 'w') as fd:
+            fd.write(mpi_spec.to_yaml(hash=ht.full_hash))
+
+        install('--no-add', '-f', mpi_spec_yaml_path.strpath)
+        assert(mpi_spec not in e.roots())
+
+        find_output = find('-l', output=str)
+        assert(mpi_spec.name in find_output)
+
+        # Without "--no-add", install an unambiguous depependency spec (that
+        # already exists as a dep in the environment) without --no-add and make
+        # sure it is added as a root of the environment as well as installed.
+        assert(b_spec not in e.roots())
+
+        install('b')
+
+        assert(b_spec in e.roots())
+        assert(b_spec not in e.uninstalled_specs())
+
+        # Without "--no-add", install a novel spec and make sure it is added
+        # as a root and installed.
+        install('bowtie')
+
+        assert(any([s.name == 'bowtie' for s in e.roots()]))
+        assert(not any([s.name == 'bowtie' for s in e.uninstalled_specs()]))
+
+
 def test_install_help_does_not_show_cdash_options(capsys):
     """
     Make sure `spack install --help` does not describe CDash arguments

diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash
@@ -1063,7 +1063,7 @@ _spack_info() {
 _spack_install() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help --only -u --until -j --jobs --overwrite --fail-fast --keep-prefix --keep-stage --dont-restage --use-cache --no-cache --cache-only --monitor --monitor-no-auth --monitor-keep-going --monitor-host --monitor-prefix --include-build-deps --no-check-signature --require-full-hash-match --show-log-on-error --source -n --no-checksum --deprecated -v --verbose --fake --only-concrete -f --file --clean --dirty --test --run-tests --log-format --log-file --help-cdash --cdash-upload-url --cdash-build --cdash-site --cdash-track --cdash-buildstamp -y --yes-to-all"
+        SPACK_COMPREPLY="-h --help --only -u --until -j --jobs --overwrite --fail-fast --keep-prefix --keep-stage --dont-restage --use-cache --no-cache --cache-only --monitor --monitor-no-auth --monitor-keep-going --monitor-host --monitor-prefix --include-build-deps --no-check-signature --require-full-hash-match --show-log-on-error --source -n --no-checksum --deprecated -v --verbose --fake --only-concrete --no-add -f --file --clean --dirty --test --run-tests --log-format --log-file --help-cdash --cdash-upload-url --cdash-build --cdash-site --cdash-track --cdash-buildstamp -y --yes-to-all"
     else
         _all_packages
     fi

diff --git a/var/spack/repos/builtin.mock/packages/b/package.py b/var/spack/repos/builtin.mock/packages/b/package.py
@@ -13,5 +13,6 @@ class B(Package):
     url      = "http://www.example.com/b-1.0.tar.gz"
 
     version('1.0', '0123456789abcdef0123456789abcdef')
+    version('0.9', 'abcd456789abcdef0123456789abcdef')
 
     depends_on('test-dependency', type='test')