From ed9d49591545c3c3b9286345f89fd9929235ecfd Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Mon, 22 Jan 2024 20:39:12 +0100 Subject: [PATCH] environment.py: drop early exit in install (#42145) `spack install` early exit behavior was sometimes convenient, except that it had and has bugs: 1. prior bug: we didn't mark env roots of already installed specs as explicit in the db 2. current bug: `spack install --overwrite` is ignored So this PR simplifies by letting the installer do its thing even if everything is supposedly installed. --- lib/spack/spack/environment/environment.py | 55 +++++----------------- lib/spack/spack/test/cmd/env.py | 14 ------ 2 files changed, 12 insertions(+), 57 deletions(-) diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index bbdc549cc2..8566244a49 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -1803,8 +1803,8 @@ def _add_concrete_spec(self, spec, concrete, new=True): self.concretized_order.append(h) self.specs_by_hash[h] = concrete - def _get_overwrite_specs(self): - # Find all dev specs that were modified. + def _dev_specs_that_need_overwrite(self): + """Return the hashes of all specs that need to be reinstalled due to source code change.""" changed_dev_specs = [ s for s in traverse.traverse_nodes( @@ -1862,52 +1862,21 @@ def install_all(self, **install_args): """ self.install_specs(None, **install_args) - def install_specs(self, specs=None, **install_args): - 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. As a small optimization, drop already installed root specs. - installed_roots, uninstalled_roots = self._partition_roots_by_install_status() - if specs: - specs_to_install = [s for s in specs if s not in installed_roots] - specs_dropped = [s for s in specs if s in installed_roots] - else: - specs_to_install = uninstalled_roots - specs_dropped = installed_roots + def install_specs(self, specs: Optional[List[Spec]] = None, **install_args): + roots = self.concrete_roots() + specs = specs if specs is not None else roots - # We need to repeat the work of the installer thanks to the above optimization: - # Already installed root specs should be marked explicitly installed in the - # database. - if specs_dropped: - with spack.store.STORE.db.write_transaction(): # do all in one transaction - for spec in specs_dropped: - spack.store.STORE.db.update_explicit(spec, True) + # Extend the set of specs to overwrite with modified dev specs and their parents + install_args["overwrite"] = ( + install_args.get("overwrite", []) + self._dev_specs_that_need_overwrite() + ) - if not specs_to_install: - tty.msg("All of the packages are already installed") - else: - tty.debug("Processing {0} uninstalled specs".format(len(specs_to_install))) - - specs_to_overwrite = self._get_overwrite_specs() - tty.debug("{0} specs need to be overwritten".format(len(specs_to_overwrite))) - - install_args["overwrite"] = install_args.get("overwrite", []) + specs_to_overwrite - - installs = [] - for spec in specs_to_install: - pkg_install_args = install_args.copy() - pkg_install_args["explicit"] = spec in self.roots() - installs.append((spec.package, pkg_install_args)) + installs = [(spec.package, {**install_args, "explicit": spec in roots}) for spec in specs] try: - builder = PackageInstaller(installs) - builder.install() + PackageInstaller(installs).install() finally: - # Ensure links are set appropriately - for spec in specs_to_install: - if spec.installed: - self.new_installs.append(spec) + self.new_installs.extend(s for s in specs if s.installed) def all_specs_generator(self) -> Iterable[Spec]: """Returns a generator for all concrete specs""" diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index 1d3380b8a6..f3da11d1d6 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -303,20 +303,6 @@ def test_activate_adds_transitive_run_deps_to_path(install_mockery, mock_fetch, assert env_variables["DEPENDENCY_ENV_VAR"] == "1" -def test_env_install_same_spec_twice(install_mockery, mock_fetch): - env("create", "test") - - e = ev.read("test") - with e: - # The first installation outputs the package prefix, updates the view - out = install("--add", "cmake-client") - assert "Updating view at" in out - - # The second installation reports all packages already installed - out = install("cmake-client") - assert "already installed" in out - - def test_env_definition_symlink(install_mockery, mock_fetch, tmpdir): filepath = str(tmpdir.join("spack.yaml")) filepath_mid = str(tmpdir.join("spack_mid.yaml"))