Don't delete "spack develop" build artifacts after install (#43424)

After #41373, where we stopped considering the source directory to be the stage for develop builds,
we resumed *deleting* the stage even after a successful build.

We don't want this for develop builds because developers need to iterate; we should keep the artifacts
unless they explicitly run `spack clean`.  

Now:
- [x] Build artifacts for develop packages are not removed after a successful install
- [x] They are also not removed before an install starts, i.e. develop packages always 
      reuse prior artifacts, if available.
- [x] They can be deleted in any other context, e.g. by running  `spack clean --stage`

lib/spack/spack/installer.py

@@ -2274,11 +2274,15 @@ def __init__(self, pkg: "spack.package_base.PackageBase", install_args: dict):
         # whether to install source code with the packag
         self.install_source = install_args.get("install_source", False)
 
+        is_develop = pkg.spec.is_develop
         # whether to keep the build stage after installation
-        self.keep_stage = install_args.get("keep_stage", False)
+        # Note: user commands do not have an explicit choice to disable
-
+        # keeping stages (i.e., we have a --keep-stage option, but not
+        # a --destroy-stage option), so we can override a default choice
+        # to destroy
+        self.keep_stage = is_develop or install_args.get("keep_stage", False)
         # whether to restage
-        self.restage = install_args.get("restage", False)
+        self.restage = (not is_develop) and install_args.get("restage", False)
 
         # whether to skip the patch phase
         self.skip_patch = install_args.get("skip_patch", False)

lib/spack/spack/spec.py

@@ -1408,6 +1408,13 @@ def external_path(self, ext_path):
     def external(self):
         return bool(self.external_path) or bool(self.external_modules)
 
+    @property
+    def is_develop(self):
+        """Return whether the Spec represents a user-developed package
+        in a Spack ``Environment`` (i.e. using `spack develop`).
+        """
+        return bool(self.variants.get("dev_path", False))
+
     def clear_dependencies(self):
         """Trim the dependencies of this spec."""
         self._dependencies.clear()

lib/spack/spack/stage.py

@@ -927,6 +927,10 @@ def destroy(self):
             shutil.rmtree(self.path)
         except FileNotFoundError:
             pass
+        try:
+            os.remove(self.reference_link)
+        except FileNotFoundError:
+            pass
         self.created = False
 
     def restage(self):

lib/spack/spack/test/cmd/dev_build.py

@@ -20,7 +20,10 @@
 install = SpackCommand("install")
 env = SpackCommand("env")
 
-pytestmark = pytest.mark.not_on_windows("does not run on windows")
+pytestmark = [
+    pytest.mark.not_on_windows("does not run on windows"),
+    pytest.mark.disable_clean_stage_check,
+]
 
 
 def test_dev_build_basics(tmpdir, install_mockery):
@@ -41,7 +44,6 @@ def test_dev_build_basics(tmpdir, install_mockery):
     assert os.path.exists(str(tmpdir))
 
 
-@pytest.mark.disable_clean_stage_check
 def test_dev_build_before(tmpdir, install_mockery):
     spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
 
@@ -58,7 +60,6 @@ def test_dev_build_before(tmpdir, install_mockery):
     assert not os.path.exists(spec.prefix)
 
 
-@pytest.mark.disable_clean_stage_check
 def test_dev_build_until(tmpdir, install_mockery):
     spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
 
@@ -76,7 +77,6 @@ def test_dev_build_until(tmpdir, install_mockery):
     assert not spack.store.STORE.db.query(spec, installed=True)
 
 
-@pytest.mark.disable_clean_stage_check
 def test_dev_build_until_last_phase(tmpdir, install_mockery):
     # Test that we ignore the last_phase argument if it is already last
     spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
@@ -96,7 +96,6 @@ def test_dev_build_until_last_phase(tmpdir, install_mockery):
     assert os.path.exists(str(tmpdir))
 
 
-@pytest.mark.disable_clean_stage_check
 def test_dev_build_before_until(tmpdir, install_mockery, capsys):
     spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
 
@@ -134,7 +133,6 @@ def mock_module_noop(*args):
     pass
 
 
-@pytest.mark.disable_clean_stage_check
 def test_dev_build_drop_in(tmpdir, mock_packages, monkeypatch, install_mockery, working_env):
     monkeypatch.setattr(os, "execvp", print_spack_cc)
     monkeypatch.setattr(spack.build_environment, "module", mock_module_noop)

lib/spack/spack/test/cmd/env.py

@@ -3161,6 +3161,41 @@ def test_modules_exist_after_env_install(
                     assert spec.prefix in contents
 
 
+@pytest.mark.disable_clean_stage_check
+def test_install_develop_keep_stage(
+    environment_from_manifest, install_mockery, mock_fetch, monkeypatch, tmpdir
+):
+    """Develop a dependency of a package and make sure that the associated
+    stage for the package is retained after a successful install.
+    """
+    environment_from_manifest(
+        """
+spack:
+  specs:
+  - mpileaks
+"""
+    )
+
+    monkeypatch.setattr(spack.stage.DevelopStage, "destroy", _always_fail)
+
+    with ev.read("test") as e:
+        libelf_dev_path = tmpdir.ensure("libelf-test-dev-path", dir=True)
+        develop(f"--path={libelf_dev_path}", "libelf@0.8.13")
+        concretize()
+        (libelf_spec,) = e.all_matching_specs("libelf")
+        (mpileaks_spec,) = e.all_matching_specs("mpileaks")
+        assert not os.path.exists(libelf_spec.package.stage.path)
+        assert not os.path.exists(mpileaks_spec.package.stage.path)
+        install()
+        assert os.path.exists(libelf_spec.package.stage.path)
+        assert not os.path.exists(mpileaks_spec.package.stage.path)
+
+
+# Helper method for test_install_develop_keep_stage
+def _always_fail(cls, *args, **kwargs):
+    raise Exception("Restage or destruction of dev stage detected during install")
+
+
 @pytest.mark.regression("24148")
 def test_virtual_spec_concretize_together(tmpdir):
     # An environment should permit to concretize "mpi"

lib/spack/spack/test/stage.py

@@ -909,18 +909,20 @@ def test_develop_stage(self, develop_path, tmp_build_stage_dir):
         """
         devtree, srcdir = develop_path
         stage = DevelopStage("test-stage", srcdir, reference_link="link-to-stage")
+        assert not os.path.exists(stage.reference_link)
         stage.create()
+        assert os.path.exists(stage.reference_link)
         srctree1 = _create_tree_from_dir_recursive(stage.source_path)
         assert os.path.samefile(srctree1["link-to-stage"], stage.path)
         del srctree1["link-to-stage"]
         assert srctree1 == devtree
 
         stage.destroy()
+        assert not os.path.exists(stage.reference_link)
         # Make sure destroying the stage doesn't change anything
         # about the path
         assert not os.path.exists(stage.path)
         srctree2 = _create_tree_from_dir_recursive(srcdir)
-        del srctree2["link-to-stage"]  # Note the symlink persists but is broken
         assert srctree2 == devtree