-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Don't fail uninstallation if easy-install.pth doesn't exist #7891
Don't fail uninstallation if easy-install.pth doesn't exist #7891
Conversation
Hi @uranusjr and @pradyunsg Please take a look at the PR whenever you can. Also will there be a unit test which would need to be added in https://github.com/pypa/pip/blob/master/tests/functional/test_uninstall.py to ensure nothing else is affected? If yes, I would assume the test would be:
Which existing test can I refer to in |
The implementation looks good. A test would be needed, and
|
Also, you will need to provide some description in the news fragmemt file. One simple sentence describing the issue you fixed would be enough. The line would end up in pip’s release notes when the fix is included in a new version. |
Hi @uranusjr, Thank you for the suggestion. I wrote the following test function to try the logic out but I am facing an issue. def test_uninstall_editable_and_pip_install_and_easy_install_remove(script, data):
script.environ['SETUPTOOLS_SYS_PATH_TECHNIQUE'] = 'raw'
pkg_path = data.packages.joinpath("FSPkg")
script.pip('install', '-e', '.',
expect_stderr=True, cwd=pkg_path)
# ensure both are installed with --ignore-installed:
script.pip('install', '--ignore-installed', '.',
expect_stderr=True, cwd=pkg_path, use_module=False)
list_result = script.pip('list', '--format=json')
assert {"name": "FSPkg", "version": "0.1.dev0"} \
in json.loads(list_result.stdout)
# Remove easy-install.pth
easy_install_pth = join(script.site_packages_path, 'easy-install.pth')
os.remove(easy_install_pth)
# Uninstall both develop and install
uninstall = script.pip('uninstall', 'FSPkg', '-y') I am able to find
I saw that the contents of easy-install.pth look as follows:
So when I remove easy-install.pth as part of the unit test, pytest is unable to find pip since I think it tries to look it up in easy-install.pth but can't find it. Is there any other way to install |
Hi @uranusjr , Could you please the queries at #7891 (comment) had about how to write the test case for this PR. I tried it the way you mentioned but came across some difficulties. |
Ah, because pip itself is also installed as an editable install for tests… This sucks. One way I can think of to avoid this problem is to rename Another way would be to use |
Hi @uranusjr Thanks for the suggestion. I tried two possible scenarios of renaming
I am unsure when you said to rename the file, but I tied both possible scenarios and neither worked. Could you please expand on how exactly do I achieve your suggestion. Also I observed that the install and uninstall are done twice in a uninstall test, once for develop and once for install. Do I do the same for this test, or I perform install and uninstall only once? |
pip always writes to I think the correct test flow would be
Does this make sense? |
Thanks @uranusjr , Let me create a test case based on what you said and create a commit for it. But I had a question. After doing Is that the expected behavior? |
The entry will still be shown as installed after uninstallation because Python can still find the metadata in |
Thanks for the suggestion @uranusjr Your explanation makes sense since pip will try to find the package in all pth files, including the one I renamed earlier. I have fixed that issue and added a commit for the same. Please take a look whenever possible :) |
Pinging @uranusjr to take a look at the updated PR addressing suggestions made :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One extra thing I realised when reading the news entry. The implementation and test LGTM!
In case you missed it, I also had a quetion about testing |
Hi @uranusjr Actually I thought that you were referring for fixing the news entry only. Let me address your other question about asserting the removal of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hard work @deveshks!
Thanks for all the review comments which made this easier @uranusjr :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes and Closes #7856
I have moved the
raise UninstallationError
exception from the constructor ofUninstallPathEntries
to the start of theremove
method, and I am logging a warning and returning from theremove
method if the file does not exist