-
Notifications
You must be signed in to change notification settings - Fork 162
Revert pinning of build-time dependencies #1201
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
base: main
Are you sure you want to change the base?
Revert pinning of build-time dependencies #1201
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1201 +/- ##
=======================================
Coverage 88.91% 88.91%
=======================================
Files 2 2
Lines 406 406
Branches 44 44
=======================================
Hits 361 361
Misses 35 35
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Would you try adding PIP_CONSTRAINT
into CI/tox infra?
"setuptools>=77", | ||
"setuptools-scm[toml]>=6.2", |
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.
Nit: it's usually nice to make the specs sparse.
"setuptools>=77", | |
"setuptools-scm[toml]>=6.2", | |
"setuptools >= 77", | |
"setuptools-scm [toml] >= 6.2", |
And it's be a good idea to specify in a code comment which features of the deps require those minimum versions to keep track of why they are set and help judge if it's okay to bump them in the future.
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 haven't traced how the minimum versions got in place, but I understand the idea. We should add a comment when bumping the minimum versions in the future.
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 just noticed that I applied your changes and it immediately got reformatted to the old status by pre-commit CI running pyproject-fmt.
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.
Pyproject-fmt doesn't seem to have a configuration option to change the formatting of dependencies, so I suggest we just go with that. Or do you feel strongly about the spaces?
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 think they improve readability and so I use this style in PyPUG. Many "opinionated" tools promote weird opinions so I avoid them. For human-readable content, it's important, for generated things (like pip-tools output) less so.
Certainly! I originally intended this to go into a separate PR, but I might as well include it here. |
…ownstream packaging. see pytest-dev#1192 (comment)
Pytest-asyncio currently uses separate constraints files for the base package and the [docs] extra. This creates issues when both files specify constraints on the same dependency and that dependency is bumped by Dependabot in only one file. In that case, the build may fail due to conflicting constraints (see for example pytest-dev#986). This patch creates a unified constraints file for pytest-asyncio. There's still a second constraints file for pytest-min, which is used to test against the minimum supported version of pytest.
…than assembling a custom install_command.
This avoids building the package separately for each environment.
…d tox environment. This simplifies the GitHub Actions workflow, allows future pinning of the dependencies associated with the check, and aligns development and CI setups more closely.
e24b598
to
a8d3b70
Compare
I wouldn't recommend having the same constraint file for different envs. Sooner or later you're doomed to run into conflicts, like being unable to update Sphinx because pytest conflicts with setuptools. This is an imaginary scenario but it shows how parts of deptrees in one env might influence dep resolution in other envs. Even if they are physically separate, you're making them tightly coupled with this. |
The new changes:
|
It's true that with a single constraint file we could miss packages upgrades or have to wait longer for them. On the other hand, it would address issues with automated dependency updates like the one mentioned in my previous comment, tough. In future, I think it's desirable to move away from Wouldn't a single constraints.txt be closest to that approach? |
Pinning to specific versions in pyproject.toml makes it very hard for downstream packagers to build pytest-asyncio from source.
see #1192 (comment)