Cmds {install,wheel} should return proper error code #1162
Conversation
| @@ -261,8 +261,6 @@ def run(self, options, args): | |||
| elif self.bundle: | |||
| requirement_set.create_bundle(self.bundle_filename) | |||
| logger.notify('Created bundle in %s' % self.bundle_filename) | |||
| except PreviousBuildDirError: | |||
| return | |||
There was a problem hiding this comment.
ouch, this return was still not preventing cleaning. the finally still runs.
but a previous build dir needs to prevent cleaning.
there is a test to confirm cleaning doesn't happen in this case, but I see it's broken.
so 3 recommended changes:
- keep the except, but instead of the
return, setoptions.no_clean=True, which the finally checks for - fix the broken test https://github.com/pypa/pip/blob/develop/tests/functional/test_install_cleanup.py#L131 . for it to be worthwhile, it needs this call
write_delete_marker_file(script.venv_path/'build'). cleanup will never happen on a dir, unless it contains pip's delete marker. - log this against 1.4.X (not develop), so it can be included in 1.4.2
the reason to include in 1.4.2, is that we're still deleting what we specifically said we wouldn't.
If you don't get to this, I will probably handle it on monday.
and thanks for doing this PR. overall, much better.
There was a problem hiding this comment.
cc @dstufft about adding this to 1.4.2
There was a problem hiding this comment.
regarding #1 above, re-raise after setting options.no_clean=True
There was a problem hiding this comment.
Hi @qwcode, I just pushed a small update to the PR, let me know what you think about it
|
Thank you so much for your review @qwcode, I'll follow your instructions and improve the patch! :) |
The `pip` executable was returning a successful status code (`0`) when trying to install new packages with existing previous build directories.
Cmds {install,wheel} should return proper error code
|
and I also backported into 1.4.X |
|
Thank you, I'm just excited to do more stuff on pip, you guys rock! :) |
lock
bot
added
the
auto-locked
The
pipexecutable was returning a successful status code (0) whentrying to install new packages with existing previous build directories.
from @qwcode: the reason to include in 1.4.2 is that this also contains a fix so that a PreviousBuildDirError prevents cleaning of the previous build dir (even when the build dir has a pip delete marker; if it previously exists with a delete marker, then it is safe to assume it was intentionally left, using
--no-cleanor--no-install, so should be left intact)