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

fix: require minimum versions of depenencies #267

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Mar 26, 2021

This fixes issues where an old version of requirements are cached on a users system, causing build to break on install. Testing by manually loading old versions of packages in tox. Closes #263.

Mostly selected min versions based on changelogs or just trying old versions and slowly increasing until one passed, except for packaging, where I thought 19+ seemed fine and started with that.

@gaborbernat, I'm better with nox than tox, there might be ways to clean this up? Also, is there any nice trick to get pip load the oldest possible versions instead of the latest, without manually listing them?

@uranusjr
Copy link
Member

Also, is there any nice trick to get pip load the oldest possible versions instead of the latest

Not sure I understand this line, oldest as in like packaging 1.0?

@layday
Copy link
Member

layday commented Mar 26, 2021

Not sure I understand this line, oldest as in like packaging 1.0?

The minimum version that meets the dependency specification, as in pypa/pip#8085.

@henryiii
Copy link
Contributor Author

henryiii commented Mar 26, 2021

Yes, that's exactly what I meant, will watch that issue! If you specify a version range, it's possible a user will get the oldest possible version, so it's important for testing. If no minimum version is specified, yes, that's the oldest version on PyPI, because a user could get that if it's already installed or cached. If you don't support the lowest version you allow, you can get broken installs. If you have enough users of your package, it eventually comes up. I'd really like to test this on most packages.

@henryiii henryiii marked this pull request as ready for review March 26, 2021 14:50
@henryiii
Copy link
Contributor Author

Oops, I don't think the extra tests are running due to the way the GitHub Actions are set up to manually run each environment.

@henryiii henryiii force-pushed the henryiii/fix/minversions branch from 7d7c7e8 to f9f2e46 Compare March 26, 2021 14:55
@layday
Copy link
Member

layday commented Mar 26, 2021

Why is this only being tested on 3.5 and 3.6?

@henryiii
Copy link
Contributor Author

henryiii commented Mar 26, 2021

Because the oldest possible requirements may not work on the newer Pythons, as they generally predate them. Generally, I'd put the oldest version only here, but 2.7 and 3.5 are different enough to include both, and 3.6 might become the new minimum eventually and was trivial to add, so I added that too (but could easily remove). Might be helped by a new "minimum" solver in Pip, but maybe not, since Pip doesn't know what version was the first to support Python x.y. Even the maintainer doesn't know that till after the Python release, sometimes. ;)

@henryiii
Copy link
Contributor Author

henryiii commented Mar 26, 2021

On a newer Python, you should not have a dependency sitting around cached older than the Python version itself, so I think just checking the oldest Python supported (+/- the current special cases) is fine for this check.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

This makes the CI even slower, but is the price we have to pay for correctness I guess.

tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@henryiii henryiii force-pushed the henryiii/fix/minversions branch from f9f2e46 to fd6ae40 Compare March 27, 2021 01:30
@henryiii
Copy link
Contributor Author

I don't think this is testing was hoping it would test. By adding the min values to an extra, I was hoping it would be easier to see if it was out of date, and also to get an immediate error from the minversion test if the locked version was not allowed by the range in the install requires. But the installed versions do not seem to be the locked versions.

@layday
Copy link
Member

layday commented Mar 27, 2021

It looks like this behaviour changed sometime between pip 20.2.3 and 21.0.1; the latter does respect the extra dependency versions.

@henryiii
Copy link
Contributor Author

Intentionally? A lot changed in 20.3 with the new solver.

@layday
Copy link
Member

layday commented Mar 27, 2021

I can't say if it was intentional but I assume tox in CI calls an older version of pip with the old resolver.

@layday
Copy link
Member

layday commented Mar 27, 2021

Erm, actually, this only happens when you prefix the Python version - tox -e minversion installs the lockminversions dependencies but tox -e py39-minversion doesn't. Perhaps someone familiar with tox's internals could explain what's going on here.

@FFY00
Copy link
Member

FFY00 commented Mar 27, 2021

I think this is a good idea, though right now I don't have time to check the details, so I'll leave that to the other maintainers.
I just have one note, please use setup.cfg as the prefix, as that is the location of the core change. In our currently adopted commit message model, the prefix indicates location, not action.

@henryiii
Copy link
Contributor Author

henryiii commented Mar 28, 2021

please use setup.cfg as the prefix

I changed to that structure before unmarking as draft. (I had to dig back a bit to see if it was "setup", "setup-cfg", or "setup.cfg", but I found a commit with the latter).

@henryiii henryiii force-pushed the henryiii/fix/minversions branch from fd6ae40 to 6aeaee1 Compare March 28, 2021 21:45
setup.cfg Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@henryiii henryiii force-pushed the henryiii/fix/minversions branch from 6aeaee1 to 3453ee8 Compare March 29, 2021 18:51
@henryiii henryiii requested a review from gaborbernat March 29, 2021 20:56
tox.ini Outdated Show resolved Hide resolved
tests/requirements-min.txt Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@henryiii henryiii force-pushed the henryiii/fix/minversions branch 2 times, most recently from 34c6e91 to 46f3e72 Compare March 30, 2021 13:23
@henryiii
Copy link
Contributor Author

I've included everything except pre-building a wheel. Do we want to do that?

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

CHANGELOG.rst Outdated Show resolved Hide resolved
@henryiii henryiii force-pushed the henryiii/fix/minversions branch from 46f3e72 to f6ffda9 Compare March 30, 2021 16:16
@layday
Copy link
Member

layday commented Mar 30, 2021

LGTM, thanks for working on this.

@henryiii henryiii force-pushed the henryiii/fix/minversions branch from f6ffda9 to fef90a2 Compare March 30, 2021 16:30
This fixes issues where an old version of requirements are cached on a
users system, causing build to break on install. Testing by manually
loading old versions of packages in tox.

Co-authored-by: Bernát Gábor <[email protected]>
Co-authored-by: layday <[email protected]>
@henryiii henryiii force-pushed the henryiii/fix/minversions branch from fef90a2 to 5b7050b Compare March 30, 2021 16:36
@henryiii henryiii merged commit bb8878c into pypa:main Mar 30, 2021
@henryiii henryiii deleted the henryiii/fix/minversions branch March 30, 2021 19:20
@henryiii
Copy link
Contributor Author

Thanks everyone!

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.

fix: specify minimum requirements in setup.cfg
5 participants