-
-
Couldn't load subscription status.
- Fork 2.9k
Some tox cleanups #13841
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
Some tox cleanups #13841
Conversation
7d654ed to
a814a60
Compare
| xdist: pytest-xdist>=2.1.0 | ||
| xdist: -e . | ||
| {env:_PYTEST_TOX_EXTRA_DEP:} | ||
| # Can use the same wheel for all environments. |
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.
This change is good, just to note that in CI we always test with the wheel built with hynek/build-and-inspect-python-package:
pytest/.github/workflows/test.yml
Line 284 in fd7742e
| run: tox run -e ${{ matrix.tox_env }}-coverage --installpkg `find dist/*.tar.gz` |
I understand the motivation of the change here in tox.ini is to speed up using tox locally.
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's actually about skipping the sdist build that's significant 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.
Though, you probably want editable instead. Locally, that is. And for CI we should build the dists once like I do everywhere and pass that to tox..
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.
And for CI we should build the dists once like I do everywhere and pass that to tox..
I agree, and that what's done today already (unless I'm missing something).
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.
Right, I was reviewing from mobile and didn't check. It is integrated indeed.
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 like it but this can be improved a bit.
| xdist: -e . | ||
| {env:_PYTEST_TOX_EXTRA_DEP:} | ||
| # Can use the same wheel for all environments. | ||
| package = wheel |
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.
| package = wheel | |
| package = editable |
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 not sure what are the implications of this myself, so I'll leave it out of this PR.
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.
The implications is as I described — tox will build a full project wheel when provisioning the env with wheel and a copy of the project will be installed in site-packages/. But with editable, a special PEP 660 slim wheel is built — only the metadata will go into site-packages/ and the source will be linked.
Tox 4.0.0 was released on Dec 7, 2022, should be OK to require it. On the way, since `minversion` is deprecated, replace it with `require`. https://tox.wiki/en/latest/config.html#min_version
As far as I can see this setting no longer exists in tox 4, and probably not utilized before either, so should be safe to remove.
According to https://tox.wiki/en/stable/upgrading.html#removed-tox-ini-keys: "Isolated builds are now always used."
Since pytest wheel is "universal" (not different between Python versions or implementations or anything), the same wheel can be reused. See: https://tox.wiki/en/stable/config.html#wheel_build_env https://tox.wiki/en/stable/upgrading.html#universal-wheels
I don't think they're needed.
a814a60 to
0ed35d5
Compare
Some things I noticed while looking at the tox file reviewing @sgaist's PR #13825. Please see the commits.