WIP: "spack uninstall": don't modify env by scheibelp · Pull Request #26766 · spack/spack

scheibelp · 2021-10-15T17:43:17Z

Currently if you run spack uninstall x in an environment, and x refers to a user spec (e.g. something added with spack add), Spack will update the spack.yaml to remove that spec. This changes the behavior so that spack uninstall just uninstalls the spec: if you immediately run spack install in the environment (which in Spack is a request to install all specs registered with the environment), then - with this PR - it would reinstall the spec.

This is anticipated to be useful if a user is modifying the package.py for a package in their environment: they may want to uninstall and then reinstall it to get the latest state (at the moment, modifications to the package.py don't affect the hash that Spack uses to compute the install prefix and thereby to distinguish it from other installed specs).

spack rm can already remove specs, so there is still a command line mechanism available to edit the environment.

This could interfere with scripts that depend on the current behavior of spack uninstall, although I consider that unlikely since this I don't think users would generally script uninstall actions.

If you have environments e1 and e2, and specs [X, Y] such that [X, Y] is in e1, and [X] is in e2

spack uninstall y in e1: uninstalls the spec (but with this PR it stays in e1)
spack uninstall x in e1: complains because x is also in e2
(new) spack uninstall --remove x in e1: removes x from e1 but does not actually uninstall the spec
spack uninstall -f x in e1: uninstalls the spec (both e1 and e2 are now "missing" this spec)

If you have environments e1 and e2, and specs [P, Q, R] such that P->R, Q->R, and [P, R] are in e1 and [Q, R] are in e2

spack uninstall p in e1: uninstalls the spec
spack uninstall r in e1: will fail for two reasons: (1) P needs R in e1 and (2) e2 needs R
spack uninstall --dependents r in e1: will fail because e2 needs R
(new) spack uninstall --dependents --remove r in e1: removes R from e1 (but does not uninstall it) and uninstalls (and removes) P
[C0] spack uninstall -f --dependents r in e1: will uninstall P, Q, and R (i.e. e2 will have dependent specs uninstalled as a side effect)
(new) spack uninstall -f --dependents --remove r in e1: this uninstalls P, Q, and R, and removes [P, R] from e1
(new) spack uninstall -f --remove r in e1: uninstalls R (so it is "missing" in both environments) and removes R from e1 (note that e1 would still install R as a dependency of P, but it would no longer be listed as a root spec)

(This is now handled) This PR does not remove the complementary behavior from spack install: if a user runs spack install x in an environment, it will add x to the list of user specs recorded for the environment. This could easily be removed, although I think this behavior could be useful: without this, if you want to install a new spec to an environment, you would have to run spack add x followed by spack install.

spack install has also been updated: by default it will not modify an environment unless you specify --add (previously, you had to specify --no-add to prevent Spack from updating the current active environment).

Other notes:

If you spack uninstall -f a spec that is wanted by existing environments, their corresponding views are not updated (this was the case before, I'm just noting that explicitly here).

TODOs:

Update tests
Add test for case [C0] above
Update spack install so that it doesn't modify the active environment by default
Make the original behavior optional: add --add and --remove options to spack install and spack uninstall, respectively

…e environment: to update the spack.yaml you must call 'spack rm'.

haampie · 2021-10-15T18:05:33Z

Only read the title, but this sounds good!

haampie · 2021-10-16T10:35:53Z

lib/spack/spack/cmd/uninstall.py

+    if env:
+        env.regenerate_views()


Notice that elsewhere we do:

with self.write_transaction(): self.regenerate_views()

.. I'd be a fan not to use write_transaction, because it has the side-effect of deserializing the environment from disk, even if the in-memory representation is newer, making environments hard to use as in-memory objects in scripts where you don't need to store a spack.yaml (#25532)

not to use write_transaction, because it has the side-effect of deserializing the environment from disk

I'm not sure what you mean here: a write transaction takes a lock on the file - AFAIK it is the only way to ensure that the disk and in-memory representation are in sync (which I think is the opposite of what you are saying).

making environments hard to use as in-memory objects in scripts where you don't need to store a spack.yaml

This could be handled by either (a) do all your modifications inside of transactions (if you are interacting with disk) or (b) do none of them inside transactions. I think (a) is safer because I don't know if our API guarantees not using transactions (perhaps that is covered in #25532)

…no-add' option with an '--add' flag (which enables the old behavior)

…the env, but is not concrete, so is not found)

…her test

…(intermediate work)

iarspider · 2022-04-28T16:27:11Z

Can't wait for this to be merged.

becker33 · 2022-11-04T19:54:47Z

share/spack/spack-completion.bash

    if $list_options
    then
-        SPACK_COMPREPLY="-h --help --only -u --until -j --jobs --reuse --overwrite --fail-fast --keep-prefix --keep-stage --dont-restage --use-cache --no-cache --cache-only --monitor --monitor-save-local --monitor-tags --monitor-keep-going --monitor-host --monitor-prefix --include-build-deps --no-check-signature --require-full-hash-match --show-log-on-error --source -n --no-checksum --deprecated -v --verbose --fake --only-concrete --no-add -f --file --clean --dirty --test --run-tests --log-format --log-file --help-cdash --cdash-upload-url --cdash-build --cdash-site --cdash-track --cdash-buildstamp -y --yes-to-all"
+        SPACK_COMPREPLY="-h --help --only -u --until -j --jobs --reuse --overwrite --fail-fast --keep-prefix --keep-stage --dont-restage --use-cache --no-cache --cache-only --monitor --monitor-save-local --monitor-tags --monitor-keep-going --monitor-host --monitor-prefix --include-build-deps --no-check-signature --require-full-hash-match --show-log-on-error --source -n --no-checksum --deprecated -v --verbose --fake --only-concrete --add -f --file --clean --dirty --test --run-tests --log-format --log-file --help-cdash --cdash-upload-url --cdash-build --cdash-site --cdash-track --cdash-buildstamp -y --yes-to-all"


We should keep the --no-add flag so that people who are already using it don't have their scripts broken (it's just overspecified for the default behavior now).

becker33 · 2022-11-04T23:25:54Z

lib/spack/spack/cmd/install.py

+                        msg = ('You asked to install {0}, which is not in '
+                               'the current environment, but did not also '
+                               'specify --add'.format(abstract.name))


I think this could be clearer. Maybe

"Cannot install '{0}' because it is not in the current environment. You can add it to the environment with 'spack add {0}', or as part of the install command with 'spack install --add {0}'.format(abstract)

becker33 · 2022-11-04T23:32:25Z

lib/spack/spack/cmd/uninstall.py

+        # one or more of the specs to be uninstalled. There may also be
+        # packages in those envs which depend on the base set of packages
+        # to uninstall, but this covers that scenario.
+        or (not args.remove and spec_to_other_envs)


We should still display this message even if the removal portion will work, since the uninstall portion will not

tgamblin · 2022-11-08T00:33:32Z

Superseded by #33711.

tgamblin · 2022-11-08T07:12:53Z

This was merged as #33711.

scheibelp added 2 commits October 14, 2021 17:27

'spack uninstall' will no longer update the list of specs in an activ…

19906b4

…e environment: to update the spack.yaml you must call 'spack rm'.

refine error message if user is already specifying --dependents

df63ca3

scheibelp added the WIP label Oct 15, 2021

spackbot-app bot added the commands label Oct 15, 2021

remove now-unused import

3b12e8e

update test (and rename it based on new expected behavior)

0c3e583

spackbot-app bot added the tests General test capability(ies) label Oct 15, 2021

make sure to update views after uninstalling

5806695

haampie reviewed Oct 16, 2021

View reviewed changes

scheibelp added 16 commits October 19, 2021 10:52

intermediate work

643af3b

intermediate work 2

4823085

intermediate work 3

7900bc4

add docstring

94a2b2a

add test for 'spack uninstall --remove'

e17c4b9

change the default behavior of install to '--no-add'; replace the '--…

77c0d5a

…no-add' option with an '--add' flag (which enables the old behavior)

using 'spack install --add' where needed

bfd1476

another install command requires --add (in this case, the spec is in …

98b799c

…the env, but is not concrete, so is not found)

another install command in ci requires --add

4424162

additional install() invocations needing --add

2dad018

improper inversion of --no-add for test; forgot to add --add for anot…

15f3df9

…her test

another command needing --add

ef4bd8b

update bash completion

3646ee4

updating comments in 'spack uninstall'

cb5a24a

trying to make the implementation of get_uninstall_list more precise …

bbd754e

…(intermediate work)

fixed error handling; explicit topological sort; updated comments

e3e9eda

tgamblin requested review from alalazo, becker33 and tgamblin October 22, 2021 23:30

tgamblin self-assigned this Nov 24, 2021

scheibelp added 2 commits January 5, 2022 12:48

merge from develop (conflict with bash completion)

bb5bfdc

update bash completion (after merge)

b6b00ca

wyphan mentioned this pull request Sep 23, 2022

Proper rewrite of Spack configuration overrides (::) #32800

Open

2 tasks

scheibelp mentioned this pull request Nov 4, 2022

"spack uninstall": don't modify env #33711

Merged

becker33 requested changes Nov 4, 2022

View reviewed changes

tgamblin closed this Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: "spack uninstall": don't modify env#26766

WIP: "spack uninstall": don't modify env#26766
scheibelp wants to merge 23 commits intospack:developfrom
scheibelp:features/uninstall-dont-modify-env

scheibelp commented Oct 15, 2021 •

edited

Loading

Uh oh!

haampie commented Oct 15, 2021

Uh oh!

haampie Oct 16, 2021 •

edited

Loading

Uh oh!

scheibelp Oct 22, 2021 •

edited

Loading

Uh oh!

iarspider commented Apr 28, 2022

Uh oh!

becker33 Nov 4, 2022

Uh oh!

becker33 Nov 4, 2022

Uh oh!

becker33 Nov 4, 2022

Uh oh!

tgamblin commented Nov 8, 2022

Uh oh!

tgamblin commented Nov 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

scheibelp commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haampie commented Oct 15, 2021

Uh oh!

haampie Oct 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iarspider commented Apr 28, 2022

Uh oh!

becker33 Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

becker33 Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

becker33 Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Nov 8, 2022

Uh oh!

tgamblin commented Nov 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scheibelp commented Oct 15, 2021 •

edited

Loading

haampie Oct 16, 2021 •

edited

Loading

scheibelp Oct 22, 2021 •

edited

Loading