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

Tox -min env doesn't actually test min dependencies #5050

Closed
lafrech opened this issue Apr 11, 2023 · 6 comments
Closed

Tox -min env doesn't actually test min dependencies #5050

lafrech opened this issue Apr 11, 2023 · 6 comments
Labels

Comments

@lafrech
Copy link

lafrech commented Apr 11, 2023

Looks like tests.pallet-min.in does not do what it is meant to.

I may be confused, but AFAIU, it is meant to test on minimal supported dependencies versions.

Here's the content of tests.pallet-min.in:

Werkzeug==2.0.0
Jinja2==3.0.0
MarkupSafe==2.0.0
itsdangerous==2.0.0
click==8.0.0

And here's a job log:

py311-min: freeze> python -m pip freeze --all
py311-min: asgiref==3.6.0,attrs==22.2.0,blinker==1.5,click==8.0.0,exceptiongroup==1.1.0,Flask @ file:///home/runner/work/flask/flask/.tox/.tmp/package/1/Flask-2.3.0.dev0-py3-none-any.whl,iniconfig==2.0.0,itsdangerous==2.0.0,Jinja2==3.0.0,MarkupSafe==2.1.2,packaging==23.0,pip==23.0.1,pluggy==1.0.0,pytest==7.2.2,python-dotenv==1.0.0,setuptools==67.4.0,tomli==2.0.1,Werkzeug==2.2.3,wheel==0.38.4
py311-min: commands[0]> pytest -v --tb=short --basetemp=/home/runner/work/flask/flask/.tox/tmp/py311-min tests

The freeze indicates the versions in use are not the versions from the tests.pallet-min.in.

I believe this could be fixed with constrain_package_deps and perhaps use_frozen_constraints. I used a configuration similar to the one here in Flask in a project of mine, and I solved it using those flags, but I'm not sure how to do it in Flask's tox.ini (it may require to de-factorize the min case out of [testenv]).

dev requirements might be impacted as well. Not sure.

@davidism
Copy link
Member

davidism commented Apr 11, 2023

I'm not sure why you think the freeze indicates it's not using the requested versions. I see all some of the requested versions in that output you quoted.

@davidism
Copy link
Member

davidism commented Apr 11, 2023

Ah, I see, it's because min.in is not in sync with the actual minimum requirements. Specifically, when Flask gets installed it pulls in Werkzeug>=2.2.2. When Jinja gets installed it pulls in MarkupSafe>=2.0, so I'm not sure what's going on there. Flask doesn't actually care what version of MarkupSafe is installed though, it only imports it for an instance check.

@lafrech
Copy link
Author

lafrech commented Apr 11, 2023

IIUC, the problem is that requirements pulled-in by install override the min requirements file. The flags I mentioned have been introduced to fix this.

More on this in issue and PR.

It might not be an issue right now but the fact that the min deps file went outdated without notice is wrong, I suppose.

@davidism
Copy link
Member

davidism commented Apr 11, 2023

It's because the requirements are installed first, then the package under test (flask) is installed. Flask's min requirements are higher than the min requirements pins, so it installs newer versions. I'm having trouble understanding the documentation on the flags you mentioned, so I'd rather just update the pins for now.

@lafrech
Copy link
Author

lafrech commented Apr 11, 2023

So did I, but the issue and PR quoted above clarified the issue.

(Note that the doc is wrong: constrain_package_deps was introduced with a default of true but this was quickly changed to false to revert back to old behaviour because people were complaining: https://tox.wiki/en/latest/changelog.html#v4-4-1-2023-01-25)

Updating the pins will "hide" the issue. Flask requirements will still override the pins, so next time Flask bumps a requirement, we'll be out-of-sync again.

@davidism
Copy link
Member

OK, I've played around with the options and they make sense now. With constrain_package_deps = true, the explicitly listed dependencies of the tox env will be used as constraints when doing the package install. With use_frozen_constraints, everything installed during the deps phase will be a constraint, rather than just the explicit deps. Since we already use a full lock file, the second option doesn't really change anything. Then if a minimum requirement is changed in pyproject.toml, the tox env will fail to install unless the min lock is synced.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants