Skip to content

Ignore top_level.txt if installed-files.txt is available. #437

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 10, 2012
Merged

Ignore top_level.txt if installed-files.txt is available. #437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
104f961
Ignore top_level.txt if installed-files.txt is available.
PiDelport Jan 22, 2012
220cde1
Add a test for issue #355.
PiDelport Jul 9, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pip/req.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ def uninstall(self, auto_confirm=False):
for installed_file in dist.get_metadata('installed-files.txt').splitlines():
path = os.path.normpath(os.path.join(egg_info_path, installed_file))
paths_to_remove.add(path)
if dist.has_metadata('top_level.txt'):
elif dist.has_metadata('top_level.txt'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @pjdelport , do you know when this elif will get run? we're already inside a block that's just for for pip installs I think.
also, you might know if we have any tests that run this elif block. I don't see anything right off.
The test you added is great, but it runs through the installed-files.txt block above this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like that uninstall case would only happen if someone had done the install like so (not within pip)
"python setup.py install --single-version-externally-managed --record=<some log>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qwcode: You're right. This particular issue and test was mainly about ensuring that the elif block does not get run when it shouldn't; i don't know what circumstances it would actually be required in.

It doesn't seem to be covered by the test suite, at least: removing the whole block does not seem to induce any failures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason this came up is in pull #646, which is fix for issue #638.
switching from if to elif here exposed an issue that we needed to handle related to __pycache__ removal.
notice that I added specific import calls to your new test and others to make sure we were handling __pycache__ removal.

if dist.has_metadata('namespace_packages.txt'):
namespaces = dist.get_metadata('namespace_packages.txt')
else:
Expand Down
3 changes: 3 additions & 0 deletions tests/packages/README.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@ Version 0.2broken has a setup.py crafted to fail on install (and only on
install). If any earlier step would fail (i.e. egg-info-generation), the
already-installed version would never be uninstalled, so uninstall-rollback
would not come into play.

The parent-0.1.tar.gz and child-0.1.tar.gz packages are used by
test_uninstall:test_uninstall_overlapping_package.
Binary file added tests/packages/child-0.1.tar.gz
View file
Open in desktop
Binary file not shown.
Binary file added tests/packages/parent-0.1.tar.gz
View file
Open in desktop
Binary file not shown.
30 changes: 28 additions & 2 deletions tests/test_uninstall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import textwrap
import sys
from os.path import join
from os.path import abspath, join
from tempfile import mkdtemp
from tests.test_pip import reset_env, run_pip, assert_all_changes, write_file
from tests.test_pip import (here, reset_env, run_pip, assert_all_changes,
write_file)
from tests.local_repos import local_repo, local_checkout

from pip.util import rmtree
Expand Down Expand Up @@ -48,6 +49,31 @@ def test_uninstall_namespace_package():
assert join(env.site_packages, 'pd', 'find') in result2.files_deleted, sorted(result2.files_deleted.keys())


def test_uninstall_overlapping_package():
"""
Uninstalling a distribution that adds modules to a pre-existing package
should only remove those added modules, not the rest of the existing
package.

See: GitHub issue #355 (pip uninstall removes things it didn't install)
"""
parent_pkg = abspath(join(here, 'packages', 'parent-0.1.tar.gz'))
child_pkg = abspath(join(here, 'packages', 'child-0.1.tar.gz'))
env = reset_env()
result1 = run_pip('install', parent_pkg, expect_error=False)
assert join(env.site_packages, 'parent') in result1.files_created, sorted(result1.files_created.keys())
result2 = run_pip('install', child_pkg, expect_error=False)
assert join(env.site_packages, 'child') in result2.files_created, sorted(result2.files_created.keys())
assert join(env.site_packages, 'parent/plugins/child_plugin.py') in result2.files_created, sorted(result2.files_created.keys())
result3 = run_pip('uninstall', '-y', 'child', expect_error=False)
assert join(env.site_packages, 'child') in result3.files_deleted, sorted(result3.files_created.keys())
assert join(env.site_packages, 'parent/plugins/child_plugin.py') in result3.files_deleted, sorted(result3.files_deleted.keys())
assert join(env.site_packages, 'parent') not in result3.files_deleted, sorted(result3.files_deleted.keys())
# Additional check: uninstalling 'child' should return things to the
# previous state, without unintended side effects.
assert_all_changes(result2, result3, [])


def test_uninstall_console_scripts():
"""
Test uninstalling a package with more files (console_script entry points, extra directories).
Expand Down