Ignore top_level.txt if installed-files.txt is available. #437
Conversation
|
Fix looks correct. Pip's tests are generally fairly high-level functional tests using scripttest and running pip inside a scratch virtualenv, so the ideal test here should just match the motivating use-case as closely as possible. In other words, add two packages into tests/packages, both of which install code into the same top-level package without using a namespace package. In the test, install both packages, and then uninstall one of them and verify the other one remains. Thanks for the pull request! |
|
@pjdelport do you need a hand with the test strategy @carljm suggested? |
|
Ping - do you still need testing assistance? |
|
Pong; apologies for the high latency! I was priority interrupted for a while, there. I rebased to the current master, and added a test case. (Credit to @mithrandi for the test packages.) |
|
Merged, thanks! |
| @@ -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'): | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
lock
bot
added
the
auto-locked
This fixes #355: verified locally with Twisted plugin installs.
No new test case yet, but all existing tests pass. Any opinions on how the test for this should be automated?