Make uninstall less verbose by pradyunsg · Pull Request #4493 · pypa/pip

pradyunsg · 2017-05-17T18:28:04Z

pradyunsg · 2017-05-17T18:36:21Z

This change makes pip uninstall spew less paths when run without any flags.

$ .tox/py35/bin/pip uninstall nose
Uninstalling nose-1.3.7:
  /home/pradyunsg/code/pip/.tox/py35/bin/nosetests
  /home/pradyunsg/code/pip/.tox/py35/bin/nosetests-3.4
  /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose-1.3.7.dist-info/*
  /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/*
  /home/pradyunsg/code/pip/.tox/py35/man/man1/nosetests.1
Proceed (y/n)? y
  Successfully uninstalled nose-1.3.7

The listing of all the files (as currently given by pip uninstall) can still be gotten through the use of --verbose.

# I manually inserted the [snip] to prevent an overly long message
$ .tox/py35/bin/pip uninstall --verbose nose
Uninstalling nose-1.3.7:
  /home/pradyunsg/code/pip/.tox/py35/bin/nosetests
  /home/pradyunsg/code/pip/.tox/py35/bin/nosetests-3.4
  /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose-1.3.7.dist-info/DESCRIPTION.rst
  [snip]
  /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/usage.txt
  /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/util.py
  /home/pradyunsg/code/pip/.tox/py35/man/man1/nosetests.1
Proceed (y/n)? y
  Removing file or directory /home/pradyunsg/code/pip/.tox/py35/bin/nosetests
  Removing file or directory /home/pradyunsg/code/pip/.tox/py35/bin/nosetests-3.4
  Removing file or directory /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose-1.3.7.dist-info/DESCRIPTION.rst
  [snip]
  Removing file or directory /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/usage.txt
  Removing file or directory /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/util.py
  Removing file or directory /home/pradyunsg/code/pip/.tox/py35/man/man1/nosetests.1
  Successfully uninstalled nose-1.3.7

(edited to add descriptions to the runs)

pfmoore · 2017-05-17T20:51:53Z

Your comment is a bit confusing. I assume the first output is the new shorter version, and the one with --verbose is the same as the current default output?

One question - if there's something in /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose that won't be deleted by pip (for example, something the user added manually) then would the abbreviated output still show /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/*? That's wrong, but it seems like that's what the code does.

I know it's an extremely unlikely case, so I'm not going to make a major issue out of it if no-one else thinks it's important, but I don't like the thought that we say we're going to delete nose/* and then don't do so.

pradyunsg · 2017-05-18T05:40:04Z

I assume the first output is the new shorter version, and the one with --verbose is the same as the current default output?

Yeah... That is the output of uninstall command from this PR. Without --verbose, the output is shortened. --verbose matches the current output. (I'll edit and make it more clear)

if there's something in /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose that won't be deleted by pip (for example, something the user added manually) then would the abbreviated output still show /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/*?

It would still show it and not delete that file... I didn't think of that. I thought along the lines of what folders does a package "own" all files of, how to determine those paths and came up with this.

I don't like the thought that we say we're going to delete nose/* and then don't do so

Same but then, how do you think we should show the output then? I can't come up with something terse yet clean way of displaying this situation.

pfmoore · 2017-05-18T07:57:01Z

I think that's the problem here. There isn't a clean way to display that. Also there's the __pycache__ files to consider. IIRC, pip deletes them even though they aren't in the package's RECORD, but doesn't list them in the output of uninstall.

I don't have a good solution here. For a completely quiet uninstall, there's the -y option. If the user doesn't use that they get told what's going to happen and asked if they are OK with going ahead, so I think we need to make sure we don't mislead them about what they are agreeing to.

I guess we could add a section after the file list if there are any unexpected files, just before the "Are you sure?" prompt:

The following files appear to have been added manually and will not be deleted:
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/user-supplied.txt
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose-1.3.7.dist-info/RECORD.backup

But I suspect it'll be significantly more complex to collect that information. What do you think?

xavfernandez · 2017-05-18T08:24:00Z

+            new_paths = []
+            for path in paths:
+                if path.endswith("__init__.py"):
+                    new_paths.append(path[:-12] + os.path.sep + "*")


os.path.dirname would be cleaner :o

xavfernandez · 2017-05-18T08:33:26Z

+                path[len(shortpath.rstrip("*").rstrip(sep))] == sep
+                for shortpath in short_paths
+            )
+            if not should_add:


Maybe we should "just" enhance compact to detect the case where it has:
['a/b/toto.py', 'a/b/tutu.py'] and 'a/b' only contains the two files, then it should only keep 'a/b'
This should produce a much more compact output in most cases.

That's what I did initially but these same paths are being used to delete files and I didn't want to modify this logic because of that.

That sounds reasonable. If there are still cases where it's not compact enough, we can review those to work out what additional improvements are practical and safe. (We may need to special-case __pycache__, although https://github.com/pypa/pip/blob/master/pip/req/req_uninstall.py#L115 might cover it)

@pfmoore I added checks to filter out for .pyc files locally in the non-verbose mode.

pradyunsg · 2017-05-18T09:02:45Z

I'm gonna grab lunch now.

I think I'll just refactor out the user interaction/output code into a separate function. And do all the processing for what to show within that.

pradyunsg · 2017-05-18T11:06:12Z

But I suspect it'll be significantly more complex to collect that information. What do you think?

I think it looks good.

pradyunsg · 2017-05-18T11:08:33Z

Non-Verbose mode output now mentions which files would not be removed.

$ ./.tox/py35/bin/pip uninstall nose
Uninstalling nose-1.3.7:
  Would remove:
    /home/pradyunsg/code/pip/.tox/py35/bin/nosetests
    /home/pradyunsg/code/pip/.tox/py35/bin/nosetests-3.4
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose-1.3.7.dist-info/*
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/*
    /home/pradyunsg/code/pip/.tox/py35/man/man1/nosetests.1
  Would not remove (might be manually added):
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/foo.txt
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/foo/myawesome.txt
Proceed (y/n)? n

Verbose mode output stays the same.

$ ./.tox/py35/bin/pip uninstall nose --verbose
Uninstalling nose-1.3.7:
  Would remove:
    /home/pradyunsg/code/pip/.tox/py35/bin/nosetests
    /home/pradyunsg/code/pip/.tox/py35/bin/nosetests-3.4
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose-1.3.7.dist-info/DESCRIPTION.rst
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose-1.3.7.dist-info/INSTALLER
    [snip]
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/tools/__pycache__/nontrivial.cpython-35.pyc
    [snip]
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/usage.txt
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/util.py
    /home/pradyunsg/code/pip/.tox/py35/man/man1/nosetests.1
Proceed (y/n)? n

pradyunsg · 2017-05-18T11:10:21Z

@pfmoore @xavfernandez Do any of you think it'll be useful to add which files would not be removed to the verbose output?

I think it'll be useful there. :)

pradyunsg · 2017-05-19T04:21:37Z

IIRC, pip deletes them even though they aren't in the package's RECORD, but doesn't list them in the output of uninstall.

pip does list them in the output.

If the user doesn't use that they get told what's going to happen and asked if they are OK with going ahead, so I think we need to make sure we don't mislead them about what they are agreeing to.

I think that doing what we said we would - uninstall the package - is what we do. If the user uses -y maybe we can logger.warn them about it?

pradyunsg · 2017-05-19T04:24:36Z

I think it'll be useful there. :)

Now, after a sleep cycle, I don't think so. :/

pfmoore · 2017-05-19T07:39:40Z

I think that doing what we said we would - uninstall the package - is what we do.

What I'm getting at is that if the user doesn't specify -y or -v then we say something like

Would remove:
[...]
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/*
[...]
Proceed (y/n)?

If the user says to proceed then they are entitled to assume that the whole of /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/ will be gone. But if there's a file in there that isn't deleted by pip, we're violating that expectation.

With the addition of the extra information

  Would not remove (might be manually added):
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/foo.txt
    /home/pradyunsg/code/pip/.tox/py35/lib/python3.5/site-packages/nose/foo/myawesome.txt

it's fine, because we've explicitly said that we're going to leave some files behind.

I'm perfectly OK with the fact that if the user specified -y we just do the uninstall. I can't offhand remember if we list the files in that case, but if we do the new abbreviated form is fine - we don't need to do anything special to cover the -y case.

pradyunsg · 2017-05-19T08:04:47Z

I can't offhand remember if we list the files in that case

Files are not listed if you specify -y.

What I'm getting at is that if the user doesn't specify -y or -v then we say something like

The PR does this now. See #4493 (comment)

pradyunsg · 2017-05-19T08:06:20Z

I think this is ready.

/request-review @dstufft @pfmoore @xavfernandez

xavfernandez · 2017-05-19T22:39:11Z

+                        file_ = os.path.join(dirpath, fname)
+                        if os.path.isfile(file_) and file_ not in files:
+                            # We are skipping this file. Add it to the set.
+                            will_skip.add(file_)


@pradyunsg Ideally this logic should be extracted in a function with a few simple tests

Doing this right now.

I was thinking unittest to test that given a long list of files we do end up with the correct shorter list.

Cool. #4502 it is. :)

pradyunsg · 2017-10-05T14:47:35Z

@pfmoore Cool. I think I understand it now -- it's just the matter of running things through os.path.normcase.

pfmoore · 2017-10-05T14:51:42Z

It usually is :-) ("Just scatter normcase around" is the new version, in a post-Unicode world, of "just add a few encode/decode calls" 😃 )

lock · 2019-06-02T17:07:30Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pradyunsg added 2 commits May 17, 2017 23:56

Make uninstall less verbose

33b903e

📰

206bf1f

pradyunsg added 2 commits May 18, 2017 00:10

Compute paths later

b75f9cd

🎨

5185056

xavfernandez reviewed May 18, 2017

View reviewed changes

Move all the interaction and output logic into one method

005975b

🎨 Don't mask file builtin

32c302e

dstufft merged commit d24581f into pypa:master May 19, 2017

xavfernandez reviewed May 19, 2017

View reviewed changes

pradyunsg deleted the nicer-uninstall branch May 20, 2017 06:42

pradyunsg mentioned this pull request May 20, 2017

Add tests for shortened 'pip uninstall' output #4502

Merged

pradyunsg mentioned this pull request Oct 5, 2017

Do wheel installs from VCS/directories using ephemeral caching #4764

Merged

pradyunsg mentioned this pull request Oct 5, 2017

Normalize case before comparing #4770

Merged

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

Conversation

pradyunsg commented May 17, 2017

Uh oh!

pradyunsg commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfmoore commented May 17, 2017

Uh oh!

pradyunsg commented May 18, 2017

Uh oh!

pfmoore commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradyunsg May 18, 2017 • 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!

pradyunsg commented May 18, 2017

Uh oh!

pradyunsg commented May 18, 2017

Uh oh!

pradyunsg commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pradyunsg commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pradyunsg commented May 19, 2017

Uh oh!

pradyunsg commented May 19, 2017

Uh oh!

pfmoore commented May 19, 2017

Uh oh!

pradyunsg commented May 19, 2017

Uh oh!

pradyunsg commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradyunsg commented Oct 5, 2017

Uh oh!

pfmoore commented Oct 5, 2017

Uh oh!

lock bot commented Jun 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pradyunsg commented May 17, 2017 •

edited

Loading

pradyunsg May 18, 2017 •

edited

Loading

pradyunsg commented May 18, 2017 •

edited

Loading

pradyunsg commented May 18, 2017 •

edited

Loading