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 install packages that could mess packaging up #397
Conversation
vendor/pip-pop/pip-clean
Outdated
def main(): | ||
args = docopt(__doc__, version='pip-grep') | ||
req_file = args['<req-file>'] | ||
|
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.
Could this be streamlined?
def good_package(line):
package_name = line.split('=')[0].split('<')[0].split('>')[0]
return package_name not in BAD_PACKAGES
lines = [line for line in f if good_package(line)]
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 want to do as little parsing as possible, for fear of breaking anything with crazy requirements files.
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.
Understood... see other package names that begin with but do not end with six
for instance https://pypi.python.org/pypi?%3Aaction=search&term=Six&submit=search
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 was planning to cross that bridge when I came to it, which I honestly doubt would happen any time soon. I'll see what I can do though.
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.
@cclauss i updated the code; integrated what you suggested and made a few improvements
Should we have a message that tells people we're replacing stuff in their requirements.txt with things in the Python buildpack or overwriting their requirements.txt file at all. Also, how does this affect pipenv? |
Pipenv has this functionality built-in. I don't think a message is needed, but I could add one saying "Removing packaging libraries from requirements.txt". |
In my mind, if someone has any of these listed in their requirements.txt, it is accidental, not intentional. |
e.g. they don't understand what's going on. so, i don't want to overload them with information. |
@kennethreitz is there a place you could link them to that would help? |
No. |
This is a new problem, and one that is temporary and will go away soon (but we need to have this code for people using this version range of setuptools locally for the future). It's not well-understood or documented anywhere that I know of. |
Waiting, waiting, waiting... for CI to pass. |
bin/steps/setuptools
Outdated
# Syntax sugar. | ||
source $BIN_DIR/utils | ||
|
||
pip-clean requirements.txt |
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.
missing EOL
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!
vendor/pip-pop/pip-clean
Outdated
return package_name not in BAD_PACKAGES | ||
|
||
def main(): | ||
args = docopt(__doc__, version='pip-grep') |
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.
version='pip-grep'
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!
The more usual explanation is they have put them in because they have minimum and maximum requirements. |
Related: #393 |
|
||
|
||
def good_package(line): | ||
package_name = line.split('=')[0].split('<')[0].split('>')[0].split(' ')[0].split('#')[0].split('\n')[0] |
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.
You can just do .split()
with no parameters instead of .split(' ').split('\n') because it deals with all whitespaces. Given the for line in f
, \n is not really an issue here.
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.
\n
is included in each line
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.
Buggy. c.f. #400
Was this to work around setuptools unvendoring their dependencies in v34.0.0 (which they've then reverted in v36.0.0) and relying on |
@edmorley yes |
This reverts commit 5496c02.
No description provided.