Add recursive argument to spack develop (#46885)

* Add recursive argument to spack develop

This effort allows for a recursive develop call
which will traverse from the develop spec given back to the root(s)
and mark all packages along the path as develop.

If people are doing development across the graph then paying
fetch and full rebuild costs every time spack develop is called
is unnecessary and expensive.

Also remove the constraint for concrete specs and simply take the
max(version) if a version is not given. This should default to the
highest infinity version which is also the logical best guess for
doing development.

lib/spack/docs/environments.rst

@@ -457,6 +457,13 @@ developed package in the environment are concretized to match the
 version (and other constraints) passed as the spec argument to the
 ``spack develop`` command.
 
+When working deep in the graph it is often desirable to have multiple specs marked
+as ``develop`` so you don't have to restage and/or do full rebuilds each time you
+call ``spack install``.  The ``--recursive`` flag can be used in these scenarios
+to ensure that all the dependents of the initial spec you provide are also marked
+as develop specs.  The ``--recursive`` flag requires a pre-concretized environment
+so the graph can be traversed from the supplied spec all the way to the root specs.
+
 For packages with ``git`` attributes, git branches, tags, and commits can
 also be used as valid concrete versions (see :ref:`version-specifier`).
 This means that for a package ``foo``, ``spack develop foo@git.main`` will clone

lib/spack/spack/cmd/develop.py

@@ -3,11 +3,13 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import os
 import shutil
+from typing import Optional
 
 import llnl.util.tty as tty
 
 import spack.cmd
 import spack.config
+import spack.environment
 import spack.fetch_strategy
 import spack.repo
 import spack.spec
@@ -31,37 +33,33 @@ def setup_parser(subparser):
         "--no-clone",
         action="store_false",
         dest="clone",
-        default=None,
         help="do not clone, the package already exists at the source path",
     )
     clone_group.add_argument(
         "--clone",
         action="store_true",
         dest="clone",
-        default=None,
-        help="clone the package even if the path already exists",
+        default=True,
+        help=(
+            "(default) clone the package unless the path already exists, "
+            "use --force to overwrite"
+        ),
     )
 
     subparser.add_argument(
         "-f", "--force", help="remove any files or directories that block cloning source code"
     )
 
+    subparser.add_argument(
+        "-r",
+        "--recursive",
+        action="store_true",
+        help="traverse nodes of the graph to mark everything up to the root as a develop spec",
+    )
+
     arguments.add_common_arguments(subparser, ["spec"])
 
 
-def _update_config(spec, path):
-    find_fn = lambda section: spec.name in section
-
-    entry = {"spec": str(spec)}
-    if path != spec.name:
-        entry["path"] = path
-
-    def change_fn(section):
-        section[spec.name] = entry
-
-    spack.config.change_or_add("develop", find_fn, change_fn)
-
-
 def _retrieve_develop_source(spec: spack.spec.Spec, abspath: str) -> None:
     # "steal" the source code via staging API. We ask for a stage
     # to be created, then copy it afterwards somewhere else. It would be
@@ -83,44 +81,43 @@ def _retrieve_develop_source(spec: spack.spec.Spec, abspath: str) -> None:
     package.stage.steal_source(abspath)
 
 
-def develop(parser, args):
-    # Note: we could put develop specs in any scope, but I assume
-    # users would only ever want to do this for either (a) an active
-    # env or (b) a specified config file (e.g. that is included by
-    # an environment)
-    # TODO: when https://github.com/spack/spack/pull/35307 is merged,
-    # an active env is not required if a scope is specified
-    env = spack.cmd.require_active_env(cmd_name="develop")
-    if not args.spec:
-        if args.clone is False:
-            raise SpackError("No spec provided to spack develop command")
+def assure_concrete_spec(env: spack.environment.Environment, spec: spack.spec.Spec):
+    version = spec.versions.concrete_range_as_version
+    if not version:
+        # first check environment for a matching concrete spec
+        matching_specs = env.all_matching_specs(spec)
+        if matching_specs:
+            version = matching_specs[0].version
+            test_spec = spack.spec.Spec(f"{spec}@{version}")
+            for m_spec in matching_specs:
+                if not m_spec.satisfies(test_spec):
+                    raise SpackError(
+                        f"{spec.name}: has multiple concrete instances in the graph that can't be"
+                        " satisified by a single develop spec. To use `spack develop` ensure one"
+                        " of the following:"
+                        f"\n a) {spec.name} nodes can satisfy the same develop spec (minimally "
+                        "this means they all share the same version)"
+                        f"\n b) Provide a concrete develop spec ({spec.name}@[version]) to clearly"
+                        " indicate what should be developed"
+                    )
+        else:
+            # look up the maximum version so infintiy versions are preferred for develop
+            version = max(spec.package_class.versions.keys())
+            tty.msg(f"Defaulting to highest version: {spec.name}@{version}")
+    spec.versions = spack.version.VersionList([version])
 
-        # download all dev specs
-        for name, entry in env.dev_specs.items():
-            path = entry.get("path", name)
-            abspath = spack.util.path.canonicalize_path(path, default_wd=env.path)
 
-            if os.path.exists(abspath):
-                msg = "Skipping developer download of %s" % entry["spec"]
-                msg += " because its path already exists."
-                tty.msg(msg)
-                continue
+def setup_src_code(spec: spack.spec.Spec, src_path: str, clone: bool = True, force: bool = False):
+    """
+    Handle checking, cloning or overwriting source code
+    """
+    assert spec.versions
 
-            # Both old syntax `spack develop pkg@x` and new syntax `spack develop pkg@=x`
-            # are currently supported.
-            spec = spack.spec.parse_with_version_concrete(entry["spec"])
-            _retrieve_develop_source(spec, abspath)
+    if clone:
+        _clone(spec, src_path, force)
 
-        if not env.dev_specs:
-            tty.warn("No develop specs to download")
-
-        return
-
-    specs = spack.cmd.parse_specs(args.spec)
-    if len(specs) > 1:
-        raise SpackError("spack develop requires at most one named spec")
-
-    spec = specs[0]
+    if not clone and not os.path.exists(src_path):
+        raise SpackError(f"Provided path {src_path} does not exist")
 
     version = spec.versions.concrete_range_as_version
     if not version:
@@ -129,40 +126,114 @@ def develop(parser, args):
         tty.msg(f"Defaulting to highest version: {spec.name}@{version}")
     spec.versions = spack.version.VersionList([version])
 
-    # If user does not specify --path, we choose to create a directory in the
-    # active environment's directory, named after the spec
-    path = args.path or spec.name
-    if not os.path.isabs(path):
-        abspath = spack.util.path.canonicalize_path(path, default_wd=env.path)
-    else:
-        abspath = path
 
-    # clone default: only if the path doesn't exist
-    clone = args.clone
-    if clone is None:
-        clone = not os.path.exists(abspath)
+def _update_config(spec, path):
+    find_fn = lambda section: spec.name in section
 
-    if not clone and not os.path.exists(abspath):
-        raise SpackError("Provided path %s does not exist" % abspath)
+    entry = {"spec": str(spec)}
+    if path and path != spec.name:
+        entry["path"] = path
 
-    if clone:
-        if os.path.exists(abspath):
-            if args.force:
-                shutil.rmtree(abspath)
-            else:
-                msg = "Path %s already exists and cannot be cloned to." % abspath
-                msg += " Use `spack develop -f` to overwrite."
-                raise SpackError(msg)
+    def change_fn(section):
+        section[spec.name] = entry
 
-        _retrieve_develop_source(spec, abspath)
+    spack.config.change_or_add("develop", find_fn, change_fn)
+
+
+def update_env(
+    env: spack.environment.Environment,
+    spec: spack.spec.Spec,
+    specified_path: Optional[str] = None,
+    build_dir: Optional[str] = None,
+):
+    """
+    Update the spack.yaml file with additions or changes from a develop call
+    """
+    tty.debug(f"Updating develop config for {env.name} transactionally")
+
+    if not specified_path:
+        dev_entry = env.dev_specs.get(spec.name)
+        if dev_entry:
+            specified_path = dev_entry.get("path", None)
 
-    tty.debug("Updating develop config for {0} transactionally".format(env.name))
     with env.write_transaction():
-        if args.build_directory is not None:
+        if build_dir is not None:
             spack.config.add(
-                "packages:{}:package_attributes:build_directory:{}".format(
-                    spec.name, args.build_directory
-                ),
+                f"packages:{spec.name}:package_attributes:build_directory:{build_dir}",
                 env.scope_name,
             )
-        _update_config(spec, path)
+        # add develop spec and update path
+        _update_config(spec, specified_path)
+
+
+def _clone(spec: spack.spec.Spec, abspath: str, force: bool = False):
+    if os.path.exists(abspath):
+        if force:
+            shutil.rmtree(abspath)
+        else:
+            msg = f"Skipping developer download of {spec.name}"
+            msg += f" because its path {abspath} already exists."
+            tty.msg(msg)
+            return
+
+    # cloning can take a while and it's nice to get a message for the longer clones
+    tty.msg(f"Cloning source code for {spec}")
+    _retrieve_develop_source(spec, abspath)
+
+
+def _abs_code_path(
+    env: spack.environment.Environment, spec: spack.spec.Spec, path: Optional[str] = None
+):
+    src_path = path if path else spec.name
+    return spack.util.path.canonicalize_path(src_path, default_wd=env.path)
+
+
+def _dev_spec_generator(args, env):
+    """
+    Generator function to loop over all the develop specs based on how the command is called
+    If no specs are supplied then loop over the develop specs listed in the environment.
+    """
+    if not args.spec:
+        if args.clone is False:
+            raise SpackError("No spec provided to spack develop command")
+
+        for name, entry in env.dev_specs.items():
+            path = entry.get("path", name)
+            abspath = spack.util.path.canonicalize_path(path, default_wd=env.path)
+            # Both old syntax `spack develop pkg@x` and new syntax `spack develop pkg@=x`
+            # are currently supported.
+            spec = spack.spec.parse_with_version_concrete(entry["spec"])
+            yield spec, abspath
+    else:
+        specs = spack.cmd.parse_specs(args.spec)
+        if (args.path or args.build_directory) and len(specs) > 1:
+            raise SpackError(
+                "spack develop requires at most one named spec when using the --path or"
+                " --build-directory arguments"
+            )
+
+        for spec in specs:
+            if args.recursive:
+                concrete_specs = env.all_matching_specs(spec)
+                if not concrete_specs:
+                    tty.warn(
+                        f"{spec.name} has no matching concrete specs in the environment and "
+                        "will be skipped. `spack develop --recursive` requires a concretized"
+                        " environment"
+                    )
+                else:
+                    for s in concrete_specs:
+                        for node_spec in s.traverse(direction="parents", root=True):
+                            tty.debug(f"Recursive develop for {node_spec.name}")
+                            yield node_spec, _abs_code_path(env, node_spec, args.path)
+            else:
+                yield spec, _abs_code_path(env, spec, args.path)
+
+
+def develop(parser, args):
+    env = spack.cmd.require_active_env(cmd_name="develop")
+
+    for spec, abspath in _dev_spec_generator(args, env):
+        assure_concrete_spec(env, spec)
+        setup_src_code(spec, abspath, clone=args.clone, force=args.force)
+        update_env(env, spec, args.path, args.build_directory)

lib/spack/spack/environment/environment.py

@@ -1128,11 +1128,6 @@ def user_specs(self):
 
     @property
     def dev_specs(self):
-        if not self._dev_specs:
-            self._dev_specs = self._read_dev_specs()
-        return self._dev_specs
-
-    def _read_dev_specs(self):
         dev_specs = {}
         dev_config = spack.config.get("develop", {})
         for name, entry in dev_config.items():

lib/spack/spack/test/cmd/develop.py

@@ -16,6 +16,7 @@
 import spack.stage
 import spack.util.git
 import spack.util.path
+from spack.error import SpackError
 from spack.main import SpackCommand
 
 add = SpackCommand("add")
@@ -159,6 +160,7 @@ def check_path(stage, dest):
             # Create path to allow develop to modify env
             fs.mkdirp(abspath)
             develop("--no-clone", "-p", path, "mpich@1.0")
+            self.check_develop(e, spack.spec.Spec("mpich@=1.0"), path)
 
             # Remove path to ensure develop with no args runs staging code
             os.rmdir(abspath)
@@ -218,6 +220,40 @@ def test_develop_full_git_repo(
         assert len(commits) > 1
 
 
+def test_recursive(mutable_mock_env_path, install_mockery, mock_fetch):
+    env("create", "test")
+
+    with ev.read("test") as e:
+        add("indirect-mpich@1.0")
+        e.concretize()
+        specs = e.all_specs()
+
+        assert len(specs) > 1
+        develop("--recursive", "mpich")
+
+        expected_dev_specs = ["mpich", "direct-mpich", "indirect-mpich"]
+        for spec in expected_dev_specs:
+            assert spec in e.dev_specs
+
+
+def test_develop_fails_with_multiple_concrete_versions(
+    mutable_mock_env_path, install_mockery, mock_fetch
+):
+    env("create", "test")
+
+    with ev.read("test") as e:
+        add("indirect-mpich@1.0")
+        add("indirect-mpich@0.9")
+        e.unify = False
+        e.concretize()
+
+        with pytest.raises(SpackError) as develop_error:
+            develop("indirect-mpich", fail_on_error=True)
+
+        error_str = "has multiple concrete instances in the graph"
+        assert error_str in str(develop_error.value)
+
+
 def test_concretize_dev_path_with_at_symbol_in_env(mutable_mock_env_path, tmpdir, mock_packages):
     spec_like = "develop-test@develop"
 

share/spack/spack-completion.bash

@@ -994,7 +994,7 @@ _spack_dev_build() {
 _spack_develop() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -p --path -b --build-directory --no-clone --clone -f --force"
+        SPACK_COMPREPLY="-h --help -p --path -b --build-directory --no-clone --clone -f --force -r --recursive"
     else
         _all_packages
     fi

share/spack/spack-completion.fish

@@ -1430,7 +1430,7 @@ complete -c spack -n '__fish_spack_using_command dev-build' -l deprecated -f -a
 complete -c spack -n '__fish_spack_using_command dev-build' -l deprecated -d 'allow concretizer to select deprecated versions'
 
 # spack develop
-set -g __fish_spack_optspecs_spack_develop h/help p/path= b/build-directory= no-clone clone f/force=
+set -g __fish_spack_optspecs_spack_develop h/help p/path= b/build-directory= no-clone clone f/force= r/recursive
 complete -c spack -n '__fish_spack_using_command_pos_remainder 0 develop' -f -k -a '(__fish_spack_specs_or_id)'
 complete -c spack -n '__fish_spack_using_command develop' -s h -l help -f -a help
 complete -c spack -n '__fish_spack_using_command develop' -s h -l help -d 'show this help message and exit'
@@ -1441,9 +1441,11 @@ complete -c spack -n '__fish_spack_using_command develop' -s b -l build-director
 complete -c spack -n '__fish_spack_using_command develop' -l no-clone -f -a clone
 complete -c spack -n '__fish_spack_using_command develop' -l no-clone -d 'do not clone, the package already exists at the source path'
 complete -c spack -n '__fish_spack_using_command develop' -l clone -f -a clone
-complete -c spack -n '__fish_spack_using_command develop' -l clone -d 'clone the package even if the path already exists'
+complete -c spack -n '__fish_spack_using_command develop' -l clone -d '(default) clone the package unless the path already exists, use --force to overwrite'
 complete -c spack -n '__fish_spack_using_command develop' -s f -l force -r -f -a force
 complete -c spack -n '__fish_spack_using_command develop' -s f -l force -r -d 'remove any files or directories that block cloning source code'
+complete -c spack -n '__fish_spack_using_command develop' -s r -l recursive -f -a recursive
+complete -c spack -n '__fish_spack_using_command develop' -s r -l recursive -d 'traverse nodes of the graph to mark everything up to the root as a develop spec'
 
 # spack diff
 set -g __fish_spack_optspecs_spack_diff h/help json first a/attribute= ignore=

var/spack/repos/builtin.mock/packages/indirect-mpich/package.py

@@ -14,6 +14,7 @@ class IndirectMpich(Package):
     url = "http://www.example.com/indirect_mpich-1.0.tar.gz"
 
     version("1.0", md5="0123456789abcdef0123456789abcdef")
+    version("0.9", md5="1123456789abcdef0123456789abcdef")
 
     depends_on("mpi")
     depends_on("direct-mpich")