-
Notifications
You must be signed in to change notification settings - Fork 3.1k
deal with pre-existing build dirs #865
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
Conversation
unpack = True | ||
url = None | ||
if not os.path.exists(os.path.join(location, 'setup.py')): | ||
|
||
#if a checkout exists, it's unwise to keep going |
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.
nit add a space between commet and text. Also as per pep8 it should be a sentence with capitalization and a period at the end.
Minor nit then looks good to merge. |
this one has some controversy in it, so needs at least 3 votes IMO. |
A long time ago I changed all pip tests to use Maybe we shoud keep that "style". But it is probably going to be more difficult to write this kind of test with global test functions and Just telling you what Ian told me years ago, not saying "no" to this. |
It always bothered me that we use plain "build" name. Many projects have a "build" directory and it is confusing sometimes. Does someone have a good idea for naming this directory? |
as for the test class, that's to hold the setup/teardown for that test. and other tests could end up reusing that. as for the plain "build" name, that's just in virtualenvs (it's to be clear though, "build" as at the root of the virtualenv, and can't conflict with a project src checkout. |
I am +1 for |
logger.notify('Created bundle in %s' % self.bundle_filename) | ||
finally: | ||
# Clean up | ||
if (not options.no_clean) and ((not options.no_install) or options.download_dir): |
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.
It would be nice to have tests for the mix of these options.
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.
yes, let me look into that.
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.
I'm confused. there'seems to be logic and tests around --download/--noinstall being used together,
but currently it seems --download is the dominant option?
it seems we should just raise a command error that these options do nothing together.
I'll open a separate pull for that and block this pull on that until its sorted out.
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.
Issue #547 has a different scenario: installing a package in version X, aborting while installation, then try installing the same package in version Y. Like:
I think it should not be a problem since this pull request removes the build dir if any exception occur (and CTRL-C/ |
deal with pre-existing build dirs
redo of pull #712.
addresses issues #413, #709, #634 and #602.
the changes:
--no-clean
)--no-clean
option for those who want build dirs left for the sake of analysisthe --no-install/--no-download workflow is not affected by this. this still works as before (w/o using --no-clean)