Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot remove entries from nonexistent file #7856

Closed
niothiel opened this issue Mar 13, 2020 · 7 comments · Fixed by #7891
Closed

Cannot remove entries from nonexistent file #7856

niothiel opened this issue Mar 13, 2020 · 7 comments · Fixed by #7891
Labels
auto-locked Outdated issues that have been locked by automation C: uninstall The logic for uninstallation of packages state: awaiting PR Feature discussed, PR is needed type: bug A confirmed bug or unintended behavior

Comments

@niothiel
Copy link

niothiel commented Mar 13, 2020

  • pip version: 20.0.2
  • Python version: 3.7.6
  • Operating system: OSX

Problem

It appears that when attempting to uninstall a package that was installed in editable mode, removing it does not work correctly if the lib/python3.7/site-packages/easy-install.pth file does not exist.

Reproduction

  1. In a new folder, create a setup.py file:
from setuptools import setup

setup(
    name='repro',
    version='0.1dev',
)
  1. Create a virtualenv: python3 -m venv env
  2. Install in editable mode: env/bin/pip install -e .
  3. Manually remove the file: rm env/lib/python3.7/site-packages/easy-install.pth
  4. Attempt to uninstall the editable package: env/bin/pip uninstall repro
  5. Observe error message: ERROR: Cannot remove entries from nonexistent file /..../env/lib/python3.7/site-packages/easy-install.pth

Expected Behavior

The uninstall should complete successfully, removing the .egg-link, even if the easy-install.pth file is not found.

Notes

In the wild, there is something that happens with our internal setup that causes the egg-link, to be created, but not the easy-install.pth file. I'm still trying to track down how this happens. Regardless of this, I believe that pip should not fail to uninstall when the file easy-install.pth doesn't exist, as the operation it would have done there would have just removed the entry from the file anyway.

I am happy to provide any further detail if needed. Thanks! :)

@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Mar 13, 2020
@uranusjr
Copy link
Member

Sounds reasonable to me. The error message can continue to exist, but the uninstallation command does not need to fail. (It would help if pip has a mechanism to fail with a specific error code, but if the choice is between 1 and 0, I would prefer 0.)

@pradyunsg pradyunsg added C: uninstall The logic for uninstallation of packages state: awaiting PR Feature discussed, PR is needed type: bug A confirmed bug or unintended behavior labels Mar 15, 2020
@triage-new-issues triage-new-issues bot removed the S: needs triage Issues/PRs that need to be triaged label Mar 15, 2020
@deveshks
Copy link
Contributor

I can take this issue up and create the PR, but I have a question about the type of changes we want here.

If I understand correctly, we add easy-install.pth in files to remove here:

paths_to_remove.add_pth(easy_install_pth, './' + easy_install_egg)

So in order to fix the issue, do we want to stop checking for easy-install.pth here? Or do we check for easy-install.pth but not raise an exception if it isn't found at

if not os.path.isfile(pth_file):
raise UninstallationError(
"Cannot remove entries from nonexistent file {}".format(
pth_file)
)

@uranusjr
Copy link
Member

We would still want to remove it if possible (and to emit an error message if it does not exist), so I’d say the latter. I did not read very deep into the implementation though, so there might be another way.

@deveshks
Copy link
Contributor

deveshks commented Mar 21, 2020

Hi @uranusjr,

Okay, so do we want to emit the error message, but not cause the installation to fail by perhaps wrapping

paths_to_remove.add_pth(easy_install_pth, './' + easy_install_egg)

in a try catch?

Or instead of raising an error at

if not os.path.isfile(pth_file):
raise UninstallationError(
"Cannot remove entries from nonexistent file {}".format(
pth_file)
)

we can raise a warning using the warnings module, or just log the error and continue

@deveshks
Copy link
Contributor

Hi @uranusjr ,

If my above suggestions make sense now, I can go ahead and create a PR for the same

@uranusjr
Copy link
Member

uranusjr commented Mar 24, 2020

I like the second approach more; it’s probably enought to logger.warning the message and move on.

@deveshks
Copy link
Contributor

Thanks @uranusjr for the response. I will create a PR to do the same.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 5, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: uninstall The logic for uninstallation of packages state: awaiting PR Feature discussed, PR is needed type: bug A confirmed bug or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants