installer.py: don't dereference stage before installing from binaries (#41986)

This fixes an issue where pkg.stage throws because a patch cannot be found,
but the patch is redundant because the spec is reused from a build cache and
will be installed from existing binaries.

lib/spack/spack/installer.py

@@ -1335,7 +1335,6 @@ def _prepare_for_install(self, task: BuildTask) -> None:
         """
         install_args = task.request.install_args
         keep_prefix = install_args.get("keep_prefix")
-        restage = install_args.get("restage")
 
         # Make sure the package is ready to be locally installed.
         self._ensure_install_ready(task.pkg)
@@ -1367,10 +1366,6 @@ def _prepare_for_install(self, task: BuildTask) -> None:
                 else:
                     tty.debug(f"{task.pkg_id} is partially installed")
 
-            # Destroy the stage for a locally installed, non-DIYStage, package
-            if restage and task.pkg.stage.managed_by_spack:
-                task.pkg.stage.destroy()
-
         if (
             rec
             and installed_in_db
@@ -1691,6 +1686,10 @@ def _install_task(self, task: BuildTask, install_status: InstallStatus) -> None:
         try:
             self._setup_install_dir(pkg)
 
+            # Create stage object now and let it be serialized for the child process. That
+            # way monkeypatch in tests works correctly.
+            pkg.stage
+
             # Create a child process to do the actual installation.
             # Preserve verbosity settings across installs.
             spack.package_base.PackageBase._verbose = spack.build_environment.start_build_process(
@@ -2223,11 +2222,6 @@ def install(self) -> None:
                 if not keep_prefix and not action == InstallAction.OVERWRITE:
                     pkg.remove_prefix()
 
-                # The subprocess *may* have removed the build stage. Mark it
-                # not created so that the next time pkg.stage is invoked, we
-                # check the filesystem for it.
-                pkg.stage.created = False
-
             # Perform basic task cleanup for the installed spec to
             # include downgrading the write to a read lock
             self._cleanup_task(pkg)
@@ -2297,6 +2291,9 @@ def __init__(self, pkg: "spack.package_base.PackageBase", install_args: dict):
         # whether to keep the build stage after installation
         self.keep_stage = install_args.get("keep_stage", False)
 
+        # whether to restage
+        self.restage = install_args.get("restage", False)
+
         # whether to skip the patch phase
         self.skip_patch = install_args.get("skip_patch", False)
 
@@ -2327,9 +2324,13 @@ def __init__(self, pkg: "spack.package_base.PackageBase", install_args: dict):
     def run(self) -> bool:
         """Main entry point from ``build_process`` to kick off install in child."""
 
-        self.pkg.stage.keep = self.keep_stage
+        stage = self.pkg.stage
+        stage.keep = self.keep_stage
 
-        with self.pkg.stage:
+        if self.restage:
+            stage.destroy()
+
+        with stage:
             self.timer.start("stage")
 
             if not self.fake:

lib/spack/spack/test/install.py

@@ -12,10 +12,12 @@
 import llnl.util.filesystem as fs
 
 import spack.error
+import spack.mirror
 import spack.patch
 import spack.repo
 import spack.store
 import spack.util.spack_json as sjson
+from spack import binary_distribution
 from spack.package_base import (
     InstallError,
     PackageBase,
@@ -118,59 +120,25 @@ def remove_prefix(self):
         self.wrapped_rm_prefix()
 
 
-class MockStage:
-    def __init__(self, wrapped_stage):
-        self.wrapped_stage = wrapped_stage
-        self.test_destroyed = False
-
-    def __enter__(self):
-        self.create()
-        return self
-
-    def __exit__(self, exc_type, exc_val, exc_tb):
-        if exc_type is None:
-            self.destroy()
-
-    def destroy(self):
-        self.test_destroyed = True
-        self.wrapped_stage.destroy()
-
-    def create(self):
-        self.wrapped_stage.create()
-
-    def __getattr__(self, attr):
-        if attr == "wrapped_stage":
-            # This attribute may not be defined at some point during unpickling
-            raise AttributeError()
-        return getattr(self.wrapped_stage, attr)
-
-
 def test_partial_install_delete_prefix_and_stage(install_mockery, mock_fetch, working_env):
     s = Spec("canfail").concretized()
 
     instance_rm_prefix = s.package.remove_prefix
 
-    try:
+    s.package.remove_prefix = mock_remove_prefix
-        s.package.remove_prefix = mock_remove_prefix
+    with pytest.raises(MockInstallError):
-        with pytest.raises(MockInstallError):
+        s.package.do_install()
-            s.package.do_install()
+    assert os.path.isdir(s.package.prefix)
-        assert os.path.isdir(s.package.prefix)
+    rm_prefix_checker = RemovePrefixChecker(instance_rm_prefix)
-        rm_prefix_checker = RemovePrefixChecker(instance_rm_prefix)
+    s.package.remove_prefix = rm_prefix_checker.remove_prefix
-        s.package.remove_prefix = rm_prefix_checker.remove_prefix
 
-        # must clear failure markings for the package before re-installing it
+    # must clear failure markings for the package before re-installing it
-        spack.store.STORE.failure_tracker.clear(s, True)
+    spack.store.STORE.failure_tracker.clear(s, True)
 
-        s.package.set_install_succeed()
+    s.package.set_install_succeed()
-        s.package.stage = MockStage(s.package.stage)
+    s.package.do_install(restage=True)
-
+    assert rm_prefix_checker.removed
-        s.package.do_install(restage=True)
+    assert s.package.spec.installed
-        assert rm_prefix_checker.removed
-        assert s.package.stage.test_destroyed
-        assert s.package.spec.installed
-
-    finally:
-        s.package.remove_prefix = instance_rm_prefix
 
 
 @pytest.mark.disable_clean_stage_check
@@ -357,10 +325,8 @@ def test_partial_install_keep_prefix(install_mockery, mock_fetch, monkeypatch, w
     spack.store.STORE.failure_tracker.clear(s, True)
 
     s.package.set_install_succeed()
-    s.package.stage = MockStage(s.package.stage)
     s.package.do_install(keep_prefix=True)
     assert s.package.spec.installed
-    assert not s.package.stage.test_destroyed
 
 
 def test_second_install_no_overwrite_first(install_mockery, mock_fetch, monkeypatch):
@@ -644,3 +610,48 @@ def test_empty_install_sanity_check_prefix(
     spec = Spec("failing-empty-install").concretized()
     with pytest.raises(spack.build_environment.ChildError, match="Nothing was installed"):
         spec.package.do_install()
+
+
+def test_install_from_binary_with_missing_patch_succeeds(
+    temporary_store: spack.store.Store, mutable_config, tmp_path, mock_packages
+):
+    """If a patch is missing in the local package repository, but was present when building and
+    pushing the package to a binary cache, installation from that binary cache shouldn't error out
+    because of the missing patch."""
+    # Create a spec s with non-existing patches
+    s = Spec("trivial-install-test-package").concretized()
+    patches = ["a" * 64]
+    s_dict = s.to_dict()
+    s_dict["spec"]["nodes"][0]["patches"] = patches
+    s_dict["spec"]["nodes"][0]["parameters"]["patches"] = patches
+    s = Spec.from_dict(s_dict)
+
+    # Create an install dir for it
+    os.makedirs(os.path.join(s.prefix, ".spack"))
+    with open(os.path.join(s.prefix, ".spack", "spec.json"), "w") as f:
+        s.to_json(f)
+
+    # And register it in the database
+    temporary_store.db.add(s, directory_layout=temporary_store.layout, explicit=True)
+
+    # Push it to a binary cache
+    build_cache = tmp_path / "my_build_cache"
+    binary_distribution.push_or_raise(
+        s,
+        build_cache.as_uri(),
+        binary_distribution.PushOptions(unsigned=True, regenerate_index=True),
+    )
+
+    # Now re-install it.
+    s.package.do_uninstall()
+    assert not temporary_store.db.query_local_by_spec_hash(s.dag_hash())
+
+    # Source install: fails, we don't have the patch.
+    with pytest.raises(spack.error.SpecError, match="Couldn't find patch for package"):
+        s.package.do_install()
+
+    # Binary install: succeeds, we don't need the patch.
+    spack.mirror.add(spack.mirror.Mirror.from_local_path(str(build_cache)))
+    s.package.do_install(package_cache_only=True, dependencies_cache_only=True, unsigned=True)
+
+    assert temporary_store.db.query_local_by_spec_hash(s.dag_hash())