Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

pyproject.toml: Respect semantic versioning for requires #15727

Closed

Conversation

hardfalcon
Copy link

If poetry-core 1.6.0 and setuptools_rust 1.6.0 are good enough, there's no reason to prevent using version 1.6.1 of the same packages. Hence, change "<=1.6.0" into "<1.7".

If poetry-core 1.6.0 and setuptools_rust 1.6.0 are good enough, there's
no reason to prevent using version 1.6.1 of the same packages. Hence,
change "<=1.6.0" into "<1.7".

Signed-off-by: Pascal Ernster <git@hardfalcon.net>
@hardfalcon hardfalcon requested a review from a team as a code owner June 6, 2023 16:00
@DMRobertson
Copy link
Contributor

DMRobertson commented Jun 6, 2023

If poetry-core 1.6.0 and setuptools_rust 1.6.0 are good enough, there's no reason to prevent using version 1.6.1 of the same packages.

This is true only if you fully trust the authors of these packages to not make a patch release with a backwards incompatible change or with a bug.

As the comment above the changed line describes, we experienced some pain along these lines #13849 and #14079 with new releases of poetry-core (though I concede these were minor releases rather than patch releases of poetry-core). Rather than go through pain for a third time, we opted for the paranoid approach of an explicit, upper bound. Being honest, I'm not keen to undo that.

More broadly: what is your motivation for relaxing these upper bounds? Are you packaging Synapse somehow and finding that the requirements are getting in the way? We'd be more than happy to accept a PR that bumps the upper bound to poetry-core 1.6.1, provided that CI passes.

@hardfalcon
Copy link
Author

I understand your point, but I've checked the last few years worth of point releases of both libraries, and I couldn't find a single point release with a breaking change. The closest thing I could find are the release notes of setuptools-rust 1.1.2, but even those changes don't really look like breaking changes to me.

And yes, your guess is correct: My motivation was/is indeed that I've into this issue when packaging synapse for my private Archlinux package repo. Of course, I could simply fix this with a sed oneliner in my PKGBUILD, but I feel the "right" way to fix the issue would be this very pull request.

@@ -369,7 +369,7 @@ furo = ">=2022.12.7,<2024.0.0"
# system changes.
# We are happy to raise these upper bounds upon request,
# provided we check that it's safe to do so (i.e. that CI passes).
requires = ["poetry-core>=1.1.0,<=1.6.0", "setuptools_rust>=1.3,<=1.6.0"]
requires = ["poetry-core>=1.1.0,<1.7", "setuptools_rust>=1.3,<1.7"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pedantic I know, but semantic versioning would be <2 for both, since the minor releases are supposedly only meant to add backwards-compatible features.

The problem is that we've been burned by this (minor releases with breaking changes) in the past.

I think I'm with @DMRobertson here in that I'd rather keep these bounds defensive.
If we don't and the build system maintainers make a breaking release, this breaks the ability to easily install old PyPI packages or even old dependency-locked packages of Synapse from the git repository (because the build system is itself not locked, except by these bounds here).

I understand the pain this causes for packagers and sorry for that, but there isn't really a perfect choice here. (We're very happy for packagers to patch/sed the bounds up to fit your packaging situation in exchange, as well as to bump up the limits when released packages emerge that we can test in CI.)

@clokep
Copy link
Member

clokep commented Aug 24, 2023

I think this has been answered and we're more than willing to bump it as newer versions come out (we just did this for poetry-core 1.7.0 recently: #16152).

@clokep clokep closed this Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants