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")