This is pretty close to what I wanted for spack develop as the only mode of operation: a mapping of spec selectors to dev_path, discussed years ago.

I guess what needs discussion/alignment here is this: with this PR there are two workflows:

• declarative: put develop specs as config in spack.yaml, concretize later (and 🤞 dev_path is added as a variant...)
• imperative: select concrete specs from the command line

Both have use cases. My only use case is the imperative workflow for troubleshooting build failures. In fact I've never really used the declarative way, cause I worked around it by just putting pkg-that-fails-to-build dev_path=/path/to/somewhere directly in the spack:specs section. This PR improves my workflow cause I don't have to edit spack.yaml and wait an eternity to reconcretize and in the meantime hope that the solver only adds dev_path and makes no other changes due to {changes in reusable specs, non-determinism of package.py, ...}.

It seems that @psakievich has different ideas about what this selector spec on the command line means? I want to be able to be very precise and specify the current dag hash of a spec that fails to build, and I don't wanna deal with package names and versions and hope for one and only one match. I'm aware that as soon as you run spack develop --imperative-workflow /abc the hash is no longer /abc after the command finishes, but I don't care, and I obviously don't want or need /abc to be stored in spack.yaml config.

I'm fine with making develop specs only considered for things reachable from the roots via link/run deps. I'd prefer to make that a separate PR to this one, and leave this PR as only affecting how reconcretization is done.

I think the issue of a selector is one we should think through (specifically the part about selecting by hash but not recording the hash is worth working through) but I also think it is orthogonal to this PR.

As currently written, this PR has no effect on which packages have dev_path=* variants added nor on which specs are affected by the develop config, in neither the imperative nor the declarative workflow. I think we should keep it that way, and use other PRs to make other changes.

With that said, I think that leaves the following points to address still:

1. unified traversal of the entire environment instead of separate traversal of each spec
2. tests
3. docs

@haampie @psakievich do either of y'all disagree that these other issues can be orthogonalized?

I'm fine with making develop specs only considered for things reachable from the roots via link/run deps. I'd prefer to make that a separate PR to this one, and leave this PR as only affecting how reconcretization is done.

Agreed

I think the issue of a selector is one we should think through (specifically the part about selecting by hash but not recording the hash is worth working through) but I also think it is orthogonal to this PR.

Yeah, I think it affects this PR in what a good name for the flag is. Also raises the question whether this option should modify spack.yaml and register the "selector" as a develop spec (imo it should not).

I do think it makes sense to drop the --no-* flag, which is a no-op iiuc. You can introduce that at the point we ever change defaults, but that is unlikely to happen.

I agree that they are orthogonal and I also agree with @haampie that we really only need one flag for turning off the default behavior.

Yeah, I think it affects this PR in what a good name for the flag is. Also raises the question whether this option should modify spack.yaml and register the "selector" as a develop spec (imo it should not).

I think for this PR that is the paradigm that spack develop operates and it is fine for that. Perhaps we could/should add a flag or another command for changes that don't also add to the develop specs must section? I think for surgical operations that makes a ton of sense. For environments where you create it with the intention to do lots of development, recording those intentions in the yaml is nice.

As part of the commit variant changes @alalazo and I modified the concretizer so commit and dev_path can be specified using prefer and require.

This may remove the need to have spack develop do so many one off things that can be handled in the solve now. Related but also for a separate PR, I want this feature for the commit variant. If we just want to advance the commit for a root spec @develop it would be nice to not have to reconcretize for that operation. My thought was finalize/merge this PR and then look at that next.

@psakievich I think it would be possible to refactor this as a generic Environment mutator that calls a generic Spec mutator, with a callback for the particular change to execute on the spec. I decided that as long as this is the only feature that works this way it's not worth doing, but let's talk about extending it to commit variants once this is in.

@becker33 we need to check this error mode for this PR: #47973

Could be a blocker.

@psakievich This should not be affected -- in this PR each node calls Spec.clear_caches() between calling Spec._mark_concrete(False) and recomputing the hashes. I've tested this by round-tripping a spec and confirming the hash is unchanged after applying and then removing the dev_path variant.

I'd like to use a hash, which hits bugs:

$ spack -e . develop --apply-changes /qbgzsgw
==> Error: expected string or bytes-like object, got 'NoneType'

$ spack -e . develop --apply-changes --path ./src /qbgzsgw
==> Cloning source code for @=1.1.1/qbgzsgw  # ???
==> Error: 'NoneType' object has no attribute 'rpartition'

Another issue is that if it fails to find a match in the environment, it will still clone its sources and update spack.yaml

$ spack -e . find blt
==> Error: No package matches the query: blt

$ spack -e . develop --apply-changes --path ./src blt
==> Cloning source code for blt@=0.7.0
==> Fetching https://mirror.spack.io/_source-cache/archive/df/df8720a9cba1199d21f1d32649cebb9dddf95aa61bc3ac23f6c8a3c6b6083528.tar.gz
    [100%]    1.33 MB @   18.4 MB/s

$ tail -n4 spack.yaml 
  develop:
    blt:
      spec: blt@=0.7.0
      path: ./src

I think this mode should update spack.lock, and error if no matching spec is found. It should probably not update spack.yaml.

@haampie you cannot use an anonymous spec with the develop command, you need to say spack develop foo/abcdef instead of spack develop /abcdef.

I disagree about whether it should update spack.yaml. If this is going to be the default mode, we need it to work whether the environment has been concretized already or not. The modification to spack.yaml is what allows you to use this with an unconcretized environment, and allows you to share the environment to other developers within a team.

I disagree about whether it should update spack.yaml.

This difference is where I could see dev-build coming back into the picture? It's a bit messy at the moment, but it was designed to work outside of environments initially. I've got a PR to make them work closer together: #47492.

spack develop has always been an environment modifying command and dev-build never has. In that sense I think dev-build could be the tool for running developer commands/anonymous changes and develop is the setup/curation command.

These two statements:

you need to say spack develop foo/abcdef instead of spack develop /abcdef.

I disagree about whether it should update spack.yaml.

together don't make sense. Obviously there is no point in having a hash recorded in the develop section.

The modification to spack.yaml is what allows you to use this with an unconcretized environment, and allows you to share the environment to other developers within a team.

I have no interest in sharing, I'm using this "develop" feature only to troubleshoot build failures, which is a very legitimate use case.

As I said earlier, I think it makes a lot of sense to have two modes of operation, one declarative like the status quo (spack.yaml changes, share with others etc), and one imperative with the new flag (spack.lock changes only).

I don't think we should be making changes to the spack.lock of an environment that aren't reflected in the spack.yaml. I think that fundamentally breaks the model.

What is the failure case that you're worried about that modifying the spack.lock is incorrect? Is the issue if the user fails to spack undevelop at the end, or is there some other concern that I'm missing?

Implementation looks good to me. I would suggest to update/reorder the docs to promote the workflow we currently recommend.

In terms of naming, I thought spack develop --config-only could be another option, but I don't care too much about this aspect.