Skip to content
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

Strip up generic versions and bump requests #6778

Merged

Conversation

ulyssessouza
Copy link
Contributor

Replaces generic limitations with a next major value
Bump the minimal requests to 2.20.0

Resolves #6756

Replaces generic limitations with a next major value
Bump the minimal `requests` to 2.20.0

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>
@ulyssessouza ulyssessouza self-assigned this Jul 2, 2019
@ulyssessouza
Copy link
Contributor Author

Hello @hartwork !
Can you review this PR to check if that's a reasonable solution for your issue (#6756) ?

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

Hi! This pull request looks well intended and looser boundaries will be of value in the context of non-system/pip context.
The added boundary to mock is a problem here though and makes things worse (see comment below) so please revert that part of the patch.
Also, the boundaries for PyYAML and texttable are not loose enough for today (see comments below).

]


if sys.version_info[:2] < (3, 4):
tests_require.append('mock >= 1.0.1')
tests_require.append('mock >= 1.0.1, < 2')
Copy link

Choose a reason for hiding this comment

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

That would downgrade people with mock 2.0.0 and upstream is at 3.0.5 by now, so that change makes things worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here was just to use the current minimal boundaries to set the upper ones as it's a guaranty (as a safe move).
Then we should to bump every requirement independently (including requirements.txt so that the binary can evolve too).

Copy link

Choose a reason for hiding this comment

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

The idea here was just to use the current minimal boundaries to set the upper ones as it's a guaranty (as a safe move).

Original mock >= 1.0.1 pulled in 3.0.5 effectively so it should have been mock <4 not mock <2 to allow the same version as before.

Then we should to bump every requirement independently (including requirements.txt so that the binary can evolve too).

That sounds right for texttable and pyyaml but not for mock.
Is there a guarantee that this branch is not ending up in a release? Is there a ticked or pull request about fixing the mock dependency regression now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I'll do the respective pull requests today 😃

'docopt >= 0.6.1, < 1',
'PyYAML >= 3.10, < 5',
'requests >= 2.20.0, < 3',
'texttable >= 0.9.0, < 1',
Copy link

Choose a reason for hiding this comment

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

That still forces users to downgrade from texttable 1.6.1 and from PyYAML 5.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

@ulyssessouza
Copy link
Contributor Author

Hello @hartwork

As promissed, I did 2 PR's to bump texttable(#6792) and mock(#6791).

For the case of PyYAML, it's more complicated.
Please have a look at #6623

@ulyssessouza ulyssessouza deleted the cleanup-setup_py-versioning branch July 8, 2019 13:24
@hartwork
Copy link

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setup.py: Generic upper bounds on dependency versions are a problem to users and to security
4 participants