[issue 1548] Make pip install -e uninstall existing versions #1552

Closed
wants to merge 7 commits into
from

Projects

None yet

9 participants

@msabramo
msabramo commented Feb 10, 2014 edited

by adding an additional call to requirement.check_if_exists() to
RequirementSet.install.

Fixes GH-1548

Example output with this PR:

$ virtualenv pip_test
...

$ cd pip_test && source bin/activate

$ pip install requests==0.5.0
Downloading/unpacking requests==0.5.0
  Using download cache from /Users/marca/.pip/download-cache/http%3A%2F%2Flocalhost%3A3141%2Froot%2Fpypi%2F%2Bf%2F6dfdc1688217d774d524e056ec6605a6%2Frequests-0.5.0.tar.gz
  Running setup.py egg_info for package requests

Installing collected packages: requests
  Running setup.py install for requests

Successfully installed requests
Cleaning up...

$ python -c 'import requests; print(requests); print(requests.__version__)'
<module 'requests' from '/private/tmp/pip_test/lib/python2.7/site-packages/requests/__init__.pyc'>
0.5.0

$ pip install -e 'git+git@github.com:msabramo/pip.git@issue_1548_make_pip_install_editable_uninstall_existing_versions#egg=pip'
Obtaining pip from git+git@github.com:msabramo/pip.git@issue_1548_make_pip_install_editable_uninstall_existing_versions#egg=pip
...
Successfully installed pip
Cleaning up...

$ pip install -e ~/dev/git-repos/requests/
Obtaining file:///Users/marca/dev/git-repos/requests
  Running setup.py (path:/Users/marca/dev/git-repos/requests/setup.py) egg_info for package from file:///Users/marca/dev/git-repos/requests

Installing collected packages: requests
  Found existing installation: requests 0.5.0
    Uninstalling requests:
      Successfully uninstalled requests
  Running setup.py develop for requests

    Creating /private/tmp/pip_test/lib/python2.7/site-packages/requests.egg-link (link to .)
    Adding requests 2.0.1 to easy-install.pth file

    Installed /Users/marca/dev/git-repos/requests
Successfully installed requests
Cleaning up...

$ python -c 'import requests; print(requests); print(requests.__version__)'
<module 'requests' from '/Users/marca/dev/git-repos/requests/requests/__init__.pyc'>
2.0.1

$ pip uninstall --verbose --yes requests
Uninstalling requests:
  Removing file or directory /private/tmp/pip_test/lib/python2.7/site-packages/requests.egg-link
  Removing pth entries from /private/tmp/pip_test/lib/python2.7/site-packages/easy-install.pth:
  Removing entry: /Users/marca/dev/git-repos/requests
  Successfully uninstalled requests

$ pip uninstall --verbose --yes requests
Cannot uninstall requirement requests, not installed
Exception information:
Traceback (most recent call last):
  File "/private/tmp/pip_test/src/pip/pip/basecommand.py", line 129, in main
    status = self.run(options, args)
  File "/private/tmp/pip_test/src/pip/pip/commands/uninstall.py", line 64, in run
    requirement_set.uninstall(auto_confirm=options.yes)
  File "/private/tmp/pip_test/src/pip/pip/req/req_set.py", line 147, in uninstall
    req.uninstall(auto_confirm=auto_confirm)
  File "/private/tmp/pip_test/src/pip/pip/req/req_install.py", line 531, in uninstall
    "Cannot uninstall requirement %s, not installed" % (self.name,)
UninstallationError: Cannot uninstall requirement requests, not installed

Note that when I do pip install -e on requests above, the previous version gets found and uninstalled.

$ pip install -e ~/dev/git-repos/requests/
Obtaining file:///Users/marca/dev/git-repos/requests
  Running setup.py (path:/Users/marca/dev/git-repos/requests/setup.py) egg_info for package from file:///Users/marca/dev/git-repos/requests

Installing collected packages: requests
↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓
  Found existing installation: requests 0.5.0
    Uninstalling requests:
      Successfully uninstalled requests
↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑
  Running setup.py develop for requests

Cc: @sontek, @aconrad, @whitmo

@msabramo msabramo [issue 1548] Make pip install -e uninstall existing versions
by adding an additional call to `requirement.check_if_exists()` to
RequirementSet.install

Fixes GH-1548
41dbd97
@msabramo

I just amended this because with my previous commit, it would not uninstall the previous version if you installed the new editable with a git URL -- e.g.: $ pip install -e 'git+https://github.com/kennethreitz/requests.git#egg=requests'. I've fixed this by adding a few more lines to check_if_exists.

@msabramo

Ugh. The tests pretty much passed -- see https://travis-ci.org/pypa/pip/builds/18658167

The only problem is that the pypy build timed out with:

I'm sorry but your test run exceeded 50.0 minutes. 
@qwcode

just noting that I see this. may not get the time to look at it til this weekend.
I'll want to try to dig into the history and find any gotcha's for this.

@msabramo

Cool, thanks @qwcode! Let me know if you end up wanting tweaks, tests, squashing and rebasing, etc...

@msabramo

@qwcode: Have you had a chance to look at this?

@qwcode qwcode commented on the diff Feb 17, 2014
pip/req/req_install.py
@@ -876,6 +876,10 @@ def check_if_exists(self):
return True
else:
self.satisfied_by = pkg_resources.get_distribution(self.req)
+
+ if self.editable and self.satisfied_by:
+ self.conflicts_with = self.satisfied_by
@qwcode
qwcode Feb 17, 2014

I think I'd like to set self.satisfied_by back to None after this.
and add a comment that says something like "when installing editables, nothing pre-existing should ever satisfy"

@qwcode qwcode and 1 other commented on an outdated diff Feb 17, 2014
pip/req/req_set.py
@@ -625,6 +625,9 @@ def install(self, install_options, global_options=(), *args, **kwargs):
# distribute wasn't installed, so nothing to do
pass
+ if not self.ignore_installed:
+ requirement.check_if_exists()
+
@qwcode
qwcode Feb 17, 2014

instead of adding this, did you try removing the not req_to_install.editable from here

it looks like it would work assuming you set satisfied_by back to None like I mention above

@msabramo
msabramo Feb 23, 2014

I just tried removing this, as you suggested, in 36cffa0. In combination with the satisfied_by = None change in 5b62075, it seems to work!

@msabramo

@qwcode: Thanks for the review. I'm AFK for a few days, but I'll try to try out the ideas you have here on Friday or so.

msabramo added some commits Feb 23, 2014
@msabramo msabramo Set self.satisfied_by back to None;
Add a comment that says something like "when installing editables,
nothing pre-existing should ever satisfy".

Addresses @qwcode's comment at
pypa#1552 (comment)
5b62075
@msabramo msabramo Remove `requirement.check_if_exists()` I added in 3870586
Addresses @qwcode's comment at
pypa#1552 (comment)
36cffa0
@msabramo

@qwcode: Tried both of your suggestions and they seem to work -- see 5b62075 and 36cffa0.

If you think that this is a sound change to make, we should probably write a test for this. Let me know -- I can try to take a stab at it.

I'd love to have this change in pip -- I get a question probably at least twice a month that ends up with me telling someone to pip uninstall the offending library twice, because they got themselves into this state.

@aconrad

I like it.! It would be nice indeed to not uninstall packages twice but I've been doing it for so long now, it's ingrained deep into my muscle memory.

@whitmo
@qwcode

@msabramo can you add a test to tests/functional/test_install.py

for your editable/non-editable pair, you can use:

  • -e https://github.com/pypa/pip-test-package@0.1#egg=pip-test-package
  • tests/data/packages/pip-test-package-0.1.tar.gz
@qwcode

@carljm any historical memories on why editable installs don't uninstall previously existing non-editables? this PR would change that.

@Ivoz
Python Packaging Authority member

I think warning that a package of the same name is already installed and stopping would be a less intrusive but still useful change.

@carljm

I think the only historical reason was that the presence (and accuracy) of the #egg= fragment on editable URLs was not considered reliable, and that's the only clue pip has as to what ought to be uninstalled. I haven't dug into this PR at all, but I would assume it can only work if the #egg= fragment is present and correct (i.e. matches the installed name of the package). This may be just fine, but it should probably be noted in docs somewhere.

@carljm

(Actually I may be wrong, maybe pip already has access to the setup.py metadata from the cloned repo by this point and can use that rather than relying on #egg=. If that's the case ignore my comment - in that case the only reason this was never done is because nobody ever did it :P)

@qwcode

currently, it is dependent on the correct #egg fragment. better if it wasn't

@qwcode

warning that a package of the same name is already installed and stopping
would be a less intrusive but still useful change

I don't see this as intrusive, just what pip should be doing in the first place, to be consistent with other installs, although, I would prefer it not to be based on the egg fragment.

@msabramo

Maybe a silly question, but why does pip require the #egg= fragment at all? Could it always get the name from the setup.py metadata. I'm guessing not - curious why.

@qwcode

but why does pip require the #egg= fragment at all

I suspect it doesn't really, if we made modifications, but see #1289 for discussion.

@msabramo

@qwcode:

@msabramo can you add a test to tests/functional/test_install.py

See 3e0d727, which adds a new test called test_install_editable_uninstalls_existing

$ .tox/py27/bin/py.test --pdb -v -k test_install_editable_uninstalls_existing
============================================================================= test session starts ==============================================================================
platform darwin -- Python 2.7.6 -- py-1.4.20 -- pytest-2.5.2 -- /Users/marca/dev/git-repos/pip/.tox/py27/bin/python
collected 413 items

tests/functional/test_install.py:231: test_install_editable_uninstalls_existing PASSED

==================================================== 412 tests deselected by '-ktest_install_editable_uninstalls_existing' =====================================================
================================================================== 1 passed, 412 deselected in 14.51 seconds ===================================================================

Question: Why do other tests in test_install.py that install editables specify expect_error=True? Should I do the same in my test even though I don't see why it would have an error?

@msabramo

Do I need to specify expect_error=True as well?

I'll remove this once I find out the answer.

Python Packaging Authority member

You'd have that if you expected the command executed to have an error. It's really what it says on the box.

some commands give stderr output, but don't return a non-zero code.
expect_error=True is needed for that.

@qwcode

to be clear, I really want to see if we can change this, so the check_exists is working against the real requirement key for the editable, not the #egg= fragment. I haven't had time to dig in yet. it involves digging into the gnarliest function that pip has, prepare_files (#1526)

@msabramo

@qwcode: That would be awesome. Would that affect the code in this PR you think?

I'm wondering if we can merge this and get at least the behavior where it works if #egg= matches and then later on refine it so that it doesn't matter? Are they independent or intertwined?

@qwcode

@msabramo before merging this, I want to see if we can make it work with the real project name, and then failing that, consider merging as is. this doesn't mean I want complete #1289 before merging something.

@msabramo

@qwcode: Did you want me to try to look at working with the real project name or were you going to do this? I don't think I have as much context as you for that, but I could try -- not sure how long it would take me.

@qwcode

@msabramo I will eventually, but certainly you can go ahead yourself.

@msabramo

@qwcode: Are you willing to consider merging this as-is and then filing an issue to refine the imperfections later?

A few weeks ago (shortly before pycon I think) I looked into this a bit but never quite got it working and now I'm kind of short on time.

@msabramo

Bump. Should we consider merging this as-is, or do you want me to try to find some time to reacquaint myself with this and make it handle the things that @qwcode mentioned?

I tend to go for not being necessarily 100% perfect so that we can make incremental progress and not lose momentum.

@msabramo

I'd love to close the loop on this.

@msabramo

@dstufft, @pfmoore: I wonder if this is something that could be squeezed into pip 6?

It irks me whenever I have to tell people to pip uninstall twice to make sure both versions are gone (or when I have to do it myself - I did it earlier today in fact).

@pfmoore
Python Packaging Authority member

I don't really have much opinion on it myself. I'm not against it going in, but I've not looked at it closely enough that I'd be willing to merge it myself.

One thought. All other installs require -U or -I to uninstall an existing version. Why are we allowing -e to do so without those flags? Does pip install -I -e already do what this patch does, or is it broken?

@aconrad

This PR makes total sense to me, so +1 👍

-U or -I shouldn't be necessary since triggering the uninstall of a package doesn't necessarily require these options to be present with the pip install command. When pinning a different version requirement, pip already uninstalls the previous package without -I or -U:

$ pip install requests==2.5.0
Downloading/unpacking requests==2.5.0
...
Installing collected packages: requests
  Found existing installation: requests 0.5.0
    Uninstalling requests:
      Successfully uninstalled requests
...

When pip install is called with -e ~/dev/git-repos/requests/, it semantically means pinning a different version requirement, like if I had done pip install requests==~/dev/git-repos/requests/.

This is why I think this PR makes absolute sense and that it should be merged.

@qwcode

To say again, the hold on this from me at the time wasn't about whether the idea in general makes sense. It does make sense.

The concern was that for vcs installs, it would work based solely on the egg= fragment being correct, which is not correct in many cases, so releasing this could end up being confusing.

But, otoh, incremental improvement is also a good argument.

at least, add something to the vcs docs explaining the situation.

https://pip.pypa.io/en/latest/reference/pip_install.html#vcs-support

@aconrad

A doc update? Love it! :)

@msabramo would you mind adding some clarification in the docs about the expectations with regards to the egg= fragment? We can then later figure out how to rather use the metadata from the cloned repo and drop the egg syntax all together (if that makes sense).

@msabramo

Yeah I can take a stab at docs later

@msabramo

I'm AFK today so if anyone feels like taking a stab at docs, be my guest.

@qwcode

@msabramo reminder, I'm willing to merge with docs updates

@msabramo

Thanks it indeed fell off my radar. Will try to update with docs this week.

@qwcode qwcode commented on the diff Mar 8, 2015
tests/functional/test_install.py
+ result = script.pip_install_local(to_install)
+ assert 'Successfully installed pip-test-package' in result.stdout
+ result.assert_installed('piptestpackage', editable=False)
+
+ result = script.pip(
+ 'install', '-e',
+ '%s#egg=pip-test-package' %
+ local_checkout(
+ 'git+http://github.com/pypa/pip-test-package.git',
+ tmpdir.join("cache"),
+ ),
+ )
+ result.assert_installed('pip-test-package', with_files=['.git'])
+ assert 'Found existing installation: pip-test-package 0.1' in result.stdout
+ assert 'Uninstalling pip-test-package:' in result.stdout
+ assert 'Successfully uninstalled pip-test-package' in result.stdout
@qwcode
qwcode Mar 8, 2015

also, would be better if you could confirm the package is removed from site-packages using result.files_deleted

@qwcode qwcode commented on the diff Mar 8, 2015
tests/functional/test_install.py
@@ -228,6 +228,32 @@ def test_install_editable_from_git(script, tmpdir):
result.assert_installed('pip-test-package', with_files=['.git'])
+def test_install_editable_uninstalls_existing(data, script, tmpdir):
+ """
+ Test that installing an editable uninstalls a previously installed
@qwcode
qwcode Mar 8, 2015

a second test that deals with a non-vcs "-e " install would be ideal, to confirm that also works as we'd want.

@qwcode

as for docs, thinking we'd need a few lines added to https://pip.pypa.io/en/latest/reference/pip_install.html#editable-installs to explain what will happen, and especially that vcs will work based on the #egg= fragment.

@dstufft dstufft closed this May 18, 2016
@dstufft
Python Packaging Authority member

Accidentally closed this, reopening. Sorry!

@dstufft dstufft reopened this May 18, 2016
@BrownTruck

Hello!

As part of an effort to ease the contribution process and adopt a more standard workflow pip has switched to doing development on the master branch. However, this Pull Request was made against the develop branch so it will need to be resubmitted against master. Unfortunately, this pull request does not cleanly merge against the current master branch.

If you do nothing, this Pull Request will be automatically closed by @BrownTruck since it cannot be merged.

If this pull request is still valid, please rebase it against master (or merge master into it) and resubmit it against the master branch, closing and referencing the original Pull Request.

If you choose to rebase/merge and resubmit this Pull Request, here is an example message that you can copy and paste:

by adding an additional call to requirement.check_if_exists() to
RequirementSet.install.

Fixes GH-1548

Example output with this PR:

$ virtualenv pip_test
...

$ cd pip_test && source bin/activate

$ pip install requests==0.5.0
Downloading/unpacking requests==0.5.0
  Using download cache from /Users/marca/.pip/download-cache/http%3A%2F%2Flocalhost%3A3141%2Froot%2Fpypi%2F%2Bf%2F6dfdc1688217d774d524e056ec6605a6%2Frequests-0.5.0.tar.gz
  Running setup.py egg_info for package requests

Installing collected packages: requests
  Running setup.py install for requests

Successfully installed requests
Cleaning up...

$ python -c 'import requests; print(requests); print(requests.__version__)'
<module 'requests' from '/private/tmp/pip_test/lib/python2.7/site-packages/requests/__init__.pyc'>
0.5.0

$ pip install -e 'git+git@github.com:msabramo/pip.git@issue_1548_make_pip_install_editable_uninstall_existing_versions#egg=pip'
Obtaining pip from git+git@github.com:msabramo/pip.git@issue_1548_make_pip_install_editable_uninstall_existing_versions#egg=pip
...
Successfully installed pip
Cleaning up...

$ pip install -e ~/dev/git-repos/requests/
Obtaining file:///Users/marca/dev/git-repos/requests
  Running setup.py (path:/Users/marca/dev/git-repos/requests/setup.py) egg_info for package from file:///Users/marca/dev/git-repos/requests

Installing collected packages: requests
  Found existing installation: requests 0.5.0
    Uninstalling requests:
      Successfully uninstalled requests
  Running setup.py develop for requests

    Creating /private/tmp/pip_test/lib/python2.7/site-packages/requests.egg-link (link to .)
    Adding requests 2.0.1 to easy-install.pth file

    Installed /Users/marca/dev/git-repos/requests
Successfully installed requests
Cleaning up...

$ python -c 'import requests; print(requests); print(requests.__version__)'
<module 'requests' from '/Users/marca/dev/git-repos/requests/requests/__init__.pyc'>
2.0.1

$ pip uninstall --verbose --yes requests
Uninstalling requests:
  Removing file or directory /private/tmp/pip_test/lib/python2.7/site-packages/requests.egg-link
  Removing pth entries from /private/tmp/pip_test/lib/python2.7/site-packages/easy-install.pth:
  Removing entry: /Users/marca/dev/git-repos/requests
  Successfully uninstalled requests

$ pip uninstall --verbose --yes requests
Cannot uninstall requirement requests, not installed
Exception information:
Traceback (most recent call last):
  File "/private/tmp/pip_test/src/pip/pip/basecommand.py", line 129, in main
    status = self.run(options, args)
  File "/private/tmp/pip_test/src/pip/pip/commands/uninstall.py", line 64, in run
    requirement_set.uninstall(auto_confirm=options.yes)
  File "/private/tmp/pip_test/src/pip/pip/req/req_set.py", line 147, in uninstall
    req.uninstall(auto_confirm=auto_confirm)
  File "/private/tmp/pip_test/src/pip/pip/req/req_install.py", line 531, in uninstall
    "Cannot uninstall requirement %s, not installed" % (self.name,)
UninstallationError: Cannot uninstall requirement requests, not installed

Note that when I do pip install -e on requests above, the previous version gets found and uninstalled.

$ pip install -e ~/dev/git-repos/requests/
Obtaining file:///Users/marca/dev/git-repos/requests
  Running setup.py (path:/Users/marca/dev/git-repos/requests/setup.py) egg_info for package from file:///Users/marca/dev/git-repos/requests

Installing collected packages: requests
↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓
  Found existing installation: requests 0.5.0
    Uninstalling requests:
      Successfully uninstalled requests
↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑
  Running setup.py develop for requests

Cc: @sontek, @aconrad, @whitmo

---

*This was migrated from pypa/pip#1552 to reparent it to the ``master`` branch. Please see original pull request for any previous discussion.*
@BrownTruck BrownTruck closed this May 26, 2016
@BrownTruck

This Pull Request was closed because it cannot be automatically reparented to the master branch and it appears to have bit rotted.

Please feel free to re-open it or re-submit it if it is still valid and you have rebased it onto master or merged master into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment