From 0158fc46aa470a895371e31aca65b8e35409c738 Mon Sep 17 00:00:00 2001 From: psakievich Date: Mon, 24 Mar 2025 17:50:16 -0600 Subject: [PATCH] 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 | 7 + lib/spack/spack/cmd/develop.py | 227 ++++++++++++------ lib/spack/spack/environment/environment.py | 5 - lib/spack/spack/test/cmd/develop.py | 36 +++ share/spack/spack-completion.bash | 2 +- share/spack/spack-completion.fish | 6 +- .../packages/indirect-mpich/package.py | 1 + 7 files changed, 198 insertions(+), 86 deletions(-) diff --git a/lib/spack/docs/environments.rst b/lib/spack/docs/environments.rst index bcbd88665d6..b73918c5211 100644 --- a/lib/spack/docs/environments.rst +++ b/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 diff --git a/lib/spack/spack/cmd/develop.py b/lib/spack/spack/cmd/develop.py index 3f3e2c12dc2..e46fde7a4ad 100644 --- a/lib/spack/spack/cmd/develop.py +++ b/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) diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 68d77d78c9e..cbd808ff49e 100644 --- a/lib/spack/spack/environment/environment.py +++ b/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(): diff --git a/lib/spack/spack/test/cmd/develop.py b/lib/spack/spack/test/cmd/develop.py index 046d5e9d8fc..e0c13213376 100644 --- a/lib/spack/spack/test/cmd/develop.py +++ b/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" diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash index 33ea464d776..f2a3c7be1dc 100644 --- a/share/spack/spack-completion.bash +++ b/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 diff --git a/share/spack/spack-completion.fish b/share/spack/spack-completion.fish index 0297384f924..654b1b3b43a 100644 --- a/share/spack/spack-completion.fish +++ b/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= diff --git a/var/spack/repos/builtin.mock/packages/indirect-mpich/package.py b/var/spack/repos/builtin.mock/packages/indirect-mpich/package.py index 66bc3b8b26a..d968e64a111 100644 --- a/var/spack/repos/builtin.mock/packages/indirect-mpich/package.py +++ b/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")