Fix dev-build keep_stage behavior (#40576)

`spack dev-build` would incorrectly set `keep_stage=True` for the
entire DAG, including for non-dev specs, even though the dev specs
have a DIYStage which never deletes sources.

lib/spack/spack/package_base.py

@@ -1808,14 +1808,7 @@ def do_install(self, **kwargs):
             verbose (bool): Display verbose build output (by default,
                 suppresses it)
         """
-        # Non-transitive dev specs need to keep the dev stage and be built from
+        PackageInstaller([(self, kwargs)]).install()
-        # source every time. Transitive ones just need to be built from source.
-        dev_path_var = self.spec.variants.get("dev_path", None)
-        if dev_path_var:
-            kwargs["keep_stage"] = True
-
-        builder = PackageInstaller([(self, kwargs)])
-        builder.install()
 
     # TODO (post-34236): Update tests and all packages that use this as a
     # TODO (post-34236): package method to the routine made available to

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

@@ -9,8 +9,10 @@
 
 import llnl.util.filesystem as fs
 
+import spack.build_environment
 import spack.environment as ev
 import spack.spec
+import spack.store
 from spack.main import SpackCommand
 
 dev_build = SpackCommand("dev-build")
@@ -20,9 +22,8 @@
 pytestmark = pytest.mark.not_on_windows("does not run on windows")
 
 
-def test_dev_build_basics(tmpdir, mock_packages, install_mockery):
+def test_dev_build_basics(tmpdir, install_mockery):
-    spec = spack.spec.Spec("dev-build-test-install@0.0.0 dev_path=%s" % tmpdir)
+    spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
-    spec.concretize()
 
     assert "dev_path" in spec.variants
 
@@ -39,9 +40,8 @@ def test_dev_build_basics(tmpdir, mock_packages, install_mockery):
     assert os.path.exists(str(tmpdir))
 
 
-def test_dev_build_before(tmpdir, mock_packages, install_mockery):
+def test_dev_build_before(tmpdir, install_mockery):
-    spec = spack.spec.Spec("dev-build-test-install@0.0.0 dev_path=%s" % tmpdir)
+    spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
-    spec.concretize()
 
     with tmpdir.as_cwd():
         with open(spec.package.filename, "w") as f:
@@ -56,9 +56,8 @@ def test_dev_build_before(tmpdir, mock_packages, install_mockery):
     assert not os.path.exists(spec.prefix)
 
 
-def test_dev_build_until(tmpdir, mock_packages, install_mockery):
+def test_dev_build_until(tmpdir, install_mockery):
-    spec = spack.spec.Spec("dev-build-test-install@0.0.0 dev_path=%s" % tmpdir)
+    spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
-    spec.concretize()
 
     with tmpdir.as_cwd():
         with open(spec.package.filename, "w") as f:
@@ -74,10 +73,9 @@ def test_dev_build_until(tmpdir, mock_packages, install_mockery):
     assert not spack.store.STORE.db.query(spec, installed=True)
 
 
-def test_dev_build_until_last_phase(tmpdir, mock_packages, install_mockery):
+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("dev-build-test-install@0.0.0 dev_path=%s" % tmpdir)
+    spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
-    spec.concretize()
 
     with tmpdir.as_cwd():
         with open(spec.package.filename, "w") as f:
@@ -94,9 +92,8 @@ def test_dev_build_until_last_phase(tmpdir, mock_packages, install_mockery):
     assert os.path.exists(str(tmpdir))
 
 
-def test_dev_build_before_until(tmpdir, mock_packages, install_mockery, capsys):
+def test_dev_build_before_until(tmpdir, install_mockery, capsys):
-    spec = spack.spec.Spec("dev-build-test-install@0.0.0 dev_path=%s" % tmpdir)
+    spec = spack.spec.Spec(f"dev-build-test-install@0.0.0 dev_path={tmpdir}").concretized()
-    spec.concretize()
 
     with tmpdir.as_cwd():
         with open(spec.package.filename, "w") as f:
@@ -134,7 +131,6 @@ def mock_module_noop(*args):
 
 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)
 
     with tmpdir.as_cwd():
@@ -142,7 +138,7 @@ def test_dev_build_drop_in(tmpdir, mock_packages, monkeypatch, install_mockery,
         assert "lib/spack/env" in output
 
 
-def test_dev_build_fails_already_installed(tmpdir, mock_packages, install_mockery):
+def test_dev_build_fails_already_installed(tmpdir, install_mockery):
     spec = spack.spec.Spec("dev-build-test-install@0.0.0 dev_path=%s" % tmpdir)
     spec.concretize()
 
@@ -175,7 +171,7 @@ def test_dev_build_fails_no_version(mock_packages):
     assert "dev-build spec must have a single, concrete version" in output
 
 
-def test_dev_build_env(tmpdir, mock_packages, install_mockery, mutable_mock_env_path):
+def test_dev_build_env(tmpdir, install_mockery, mutable_mock_env_path):
     """Test Spack does dev builds for packages in develop section of env."""
     # setup dev-build-test-install package for dev build
     build_dir = tmpdir.mkdir("build")
@@ -191,7 +187,7 @@ def test_dev_build_env(tmpdir, mock_packages, install_mockery, mutable_mock_env_
     with envdir.as_cwd():
         with open("spack.yaml", "w") as f:
             f.write(
-                """\
+                f"""\
 spack:
   specs:
   - dev-build-test-install@0.0.0
@@ -199,11 +195,9 @@ def test_dev_build_env(tmpdir, mock_packages, install_mockery, mutable_mock_env_
   develop:
     dev-build-test-install:
       spec: dev-build-test-install@0.0.0
-      path: %s
+      path: {os.path.relpath(str(build_dir), start=str(envdir))}
 """
-                % os.path.relpath(str(build_dir), start=str(envdir))
             )
-
         env("create", "test", "./spack.yaml")
         with ev.read("test"):
             install()
@@ -213,9 +207,7 @@ def test_dev_build_env(tmpdir, mock_packages, install_mockery, mutable_mock_env_
         assert f.read() == spec.package.replacement_string
 
 
-def test_dev_build_env_version_mismatch(
+def test_dev_build_env_version_mismatch(tmpdir, install_mockery, mutable_mock_env_path):
-    tmpdir, mock_packages, install_mockery, mutable_mock_env_path
-):
     """Test Spack constraints concretization by develop specs."""
     # setup dev-build-test-install package for dev build
     build_dir = tmpdir.mkdir("build")
@@ -231,7 +223,7 @@ def test_dev_build_env_version_mismatch(
     with envdir.as_cwd():
         with open("spack.yaml", "w") as f:
             f.write(
-                """\
+                f"""\
 spack:
   specs:
   - dev-build-test-install@0.0.0
@@ -239,9 +231,8 @@ def test_dev_build_env_version_mismatch(
   develop:
     dev-build-test-install:
       spec: dev-build-test-install@1.1.1
-      path: %s
+      path: {build_dir}
 """
-                % build_dir
             )
 
         env("create", "test", "./spack.yaml")
@@ -250,9 +241,7 @@ def test_dev_build_env_version_mismatch(
                 install()
 
 
-def test_dev_build_multiple(
+def test_dev_build_multiple(tmpdir, install_mockery, mutable_mock_env_path, mock_fetch):
-    tmpdir, mock_packages, install_mockery, mutable_mock_env_path, mock_fetch
-):
     """Test spack install with multiple developer builds
 
     Test that only the root needs to be specified in the environment
@@ -284,20 +273,19 @@ def test_dev_build_multiple(
     with envdir.as_cwd():
         with open("spack.yaml", "w") as f:
             f.write(
-                """\
+                f"""\
 spack:
   specs:
   - dev-build-test-dependent@0.0.0
 
   develop:
     dev-build-test-install:
-      path: %s
+      path: {leaf_dir}
       spec: dev-build-test-install@=1.0.0
     dev-build-test-dependent:
       spec: dev-build-test-dependent@0.0.0
-      path: %s
+      path: {root_dir}
 """
-                % (leaf_dir, root_dir)
             )
 
         env("create", "test", "./spack.yaml")
@@ -316,9 +304,7 @@ def test_dev_build_multiple(
             assert f.read() == spec.package.replacement_string
 
 
-def test_dev_build_env_dependency(
+def test_dev_build_env_dependency(tmpdir, install_mockery, mock_fetch, mutable_mock_env_path):
-    tmpdir, mock_packages, install_mockery, mock_fetch, mutable_mock_env_path
-):
     """
     Test non-root specs in an environment are properly marked for dev builds.
     """
@@ -337,7 +323,7 @@ def test_dev_build_env_dependency(
     with envdir.as_cwd():
         with open("spack.yaml", "w") as f:
             f.write(
-                """\
+                f"""\
 spack:
   specs:
   - dependent-of-dev-build@0.0.0
@@ -345,11 +331,9 @@ def test_dev_build_env_dependency(
   develop:
     dev-build-test-install:
       spec: dev-build-test-install@0.0.0
-      path: %s
+      path: {os.path.relpath(str(build_dir), start=str(envdir))}
 """
-                % os.path.relpath(str(build_dir), start=str(envdir))
             )
-
         env("create", "test", "./spack.yaml")
         with ev.read("test"):
             # concretize in the environment to get the dev build info
@@ -371,7 +355,7 @@ def test_dev_build_env_dependency(
 
 @pytest.mark.parametrize("test_spec", ["dev-build-test-install", "dependent-of-dev-build"])
 def test_dev_build_rebuild_on_source_changes(
-    test_spec, tmpdir, mock_packages, install_mockery, mutable_mock_env_path, mock_fetch
+    test_spec, tmpdir, install_mockery, mutable_mock_env_path, mock_fetch
 ):
     """Test dev builds rebuild on changes to source code.
 
@@ -416,4 +400,4 @@ def reset_string():
             fs.touch(os.path.join(str(build_dir), "test"))
             output = install()
 
-    assert "Installing %s" % test_spec in output
+    assert f"Installing {test_spec}" in output