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

Overly restrictive version spec for wagtail dependency in setup.py #192

Open
mrmachine opened this issue Sep 25, 2018 · 6 comments
Open

Comments

@mrmachine
Copy link
Contributor

mrmachine commented Sep 25, 2018

wagtail-personalisation claims to require wagtail<2.2, but before wagtail 2.1 was out it required <2.1, etc.

I think this has been done to avoid accidental breakage, if we assume that wagtail minor releases may include backwards incompatible changes. But, it means that even if future versions of wagtail happen to be totally compatible, you must publish a new release of wagtail-personalisation in lock step with new minor wagtail releases, and we must upgrade to it (potentially bringing along other wagtail-personalisation changes we don't want/need or unintended consequences), in order for us to use the latest version of wagtail.

In general, the version spec in setup.py should only exclude versions that are confirmed to be incompatible. Usually this is:

  1. set a minimum version that has a feature that this package requires (>=N)
  2. exclude a specific version that contains a bug that breaks this package (!=N)

Less commonly:

  1. exclude future versions that we know will be incompatible (<N)

Is wagtail-personalisation actually incompatible with wagtail >=2.2?

Either way, can we get a new release that adds compatibility (if required) and removes this constraint?

@gasman
Copy link
Collaborator

gasman commented Sep 25, 2018

In general I would disagree: it's better for a package to be conservative about the versions of dependencies it accepts, and only update the record when new versions are confirmed to work. Having to wait for a new release of the package is inconvenient, but it's less disruptive than having a project break in production because there was a breaking change in a dependency that the parent package didn't anticipate.

However, in this case wagtail is not a dependency of wagtail-personalisation in the usual sense. In any sensible scenario, the version of wagtail is already fixed within a project before the developer installs wagtail-personalisation, and wagtail-personalisation is never in charge of deciding which wagtail version to install. In this case, there's no benefit to having wagtail listed in wagtail-personalisation's dependencies at all, and I'd propose removing it completely. This seems to be fairly common practice among Django packages (see for example django-rest-framework and django-compressor which don't specify Django in their dependencies).

@mrmachine
Copy link
Contributor Author

@gasman I might agree with you, if you were talking about project dependencies, but not library packages that are used in any number of different projects with any combination of other dependencies. Although, it's better to just pin every requirement to an absolute version in a flattened project dependency tree, instead.

The problem with overly restrictive (unconfirmed/future) constraints in library packages is that they make developers of projects that use said libraries completely reliant on the library maintainer to test new versions of related packages and release updates quickly (or at all). That's more work for you.

I have zero alternative options to use the latest wagtail-personalisation with the latest wagtail, even if they are fully compatible, except to wait for a new release, or fork it myself and remove the constraint.

Even in the best case scenario, when the library maintainer does update the constraint immediately, I am still forced to upgrade more of my project dependencies than may be necessary. I must also upgrade wagtail-personalisation when I upgrade wagtail, even if the two packages are actually compatible (but for the constraint). This additional churn in my project dependency tree could also bring more bugs, or unwanted wagtail-personalisation changes, or just more work updating my project to use the latest version of a dependency I didn't really want or need.

By only excluding actual dependencies that are actually known to be incompatible, project developers are much more free to resolve incompatibilities and version conflicts in their project's dependency tree in their own requirements.txt file.

I don't need every library package to protect me from unreleased future versions of all their nested dependencies. I pin all my project dependencies anyway, which is standard practice.

I don't think it's necessary to remove the minimum version constraint (or any other constraint), if it's a legitimate actual requirement. For example, does wagtail-personalisation literally work with any version of wagtail? Did none of the wagtail imports in wagtail-personalisation move between wagtail 1.x -> 2.x?

But don't block me from using wagtail-personalisation with future versions of wagtail, before it's known that they are compatible or not, and especially not for minor version numbers.

Just imagine if every library package refused to work with future/unreleased versions of all their dependencies. Unless every single package maintainer very quickly tested their package with every new version of every one of their package's dependencies and released a new version, it would very quickly be impossible to produce a consistent dependency tree at all.

And even if every library maintainer did release such updates on day 1, that would still be a LOT of unnecessary code churn in projects due to the required cascading dependency version upgrades. A lot of extra work for project and library maintainers, alike.

@gasman
Copy link
Collaborator

gasman commented Sep 25, 2018

Let me give an example... last month I had to rush out 7 new releases of Wagtail in one day, due to a bug introduced in html5lib 4.6.1 which would have caused silent data loss from rich text fields. This was an exceptional case - normally a patch/bugfix release would be an indication from the package author that the changes are minor enough not to break anything, but of course mistakes can happen. What you're proposing is to allow any unseen future version of any dependency - even if the package author has made no guarantee about backwards compatibility - which would make this sort of screw-up into a routine occurrence. Dealing with those incidents will absolutely create more work for us as package maintainers.

If we allow all future versions of dependencies by default, there's a very real probability that in the time interval between a new dependency version being released and us getting chance to test it, our package will be just plain broken for users performing a fresh installation (possibly in catastrophic or subtle ways). I think most users would consider that a worse outcome than the package simply being out of date for that time period. Perhaps for you that's an acceptable tradeoff for having a bleeding-edge up-to-date version, but in that case you're the exception to the rule, and running git master (or forking if necessary) is the right way to go.

(Again I should clarify that I'm talking about the general case here - in this particular case, I think it's fine to relax the version constraints for the wagtail package.)

@mrmachine
Copy link
Contributor Author

Yes, your package may be broken for fresh installations that do not yet have any pinned dependencies. But such an installation is not going to have an unexpected adverse effect on an existing production system.

A fresh installation such as this will be for a brand new project, and it is up to the project maintainer to test their project and satisfy themselves that their project works before pinning their dependency tree and rolling it out to production.

Plus, you're going to benefit from bug reports from end users alerting you to the fact that there's a problem in their new/development (non-production) installations, instead of having to do pre-emptive and exhaustive testing yourself of every new version of every dependency your library package uses.

I suspect that very few projects have enough resources to properly test every new version of every dependency their package uses and release updates of their packages in a timely fashion.

The impact of library maintainers pinning their dependencies to an absolute version, or disallowing future/unreleased versions, is not merely "being out of date" for a while. It's very easy for these constraints to create project dependency version conflicts that are literally impossible for project maintainers to resolve.

For example, if a project made use of another package that legitimately requires Wagtail 2.2+ and any version of wagtail-personalisation. That's over 4 months when every available version of wagtail-personalisation has been incompatible with the 2 wagtail releases and 300+ wagtail commits during that time.

Anyway, as you say, this all general, so I'm sorry for rambling on about it. I've just been bitten by this problem a few times by restrictive choices from library package maintainers, that have been difficult or impossible to work around at the project level (without forking). It's more of an issue with widely used dependencies (e.g. six or future), which have a very high probability of causing project dependency version conflicts.

Thanks for taking the time to respond, and it'd be great if you could remove that constraint from wagtail-personalisation.

mrmachine added a commit to ixc/wagtail-personalisation that referenced this issue Sep 26, 2018
@mrmachine
Copy link
Contributor Author

@gasman I've made a PR that removes the future constraint. #193

blurrah added a commit that referenced this issue Sep 26, 2018
Remove overly restrictive wagtail dependency version constraint (#192)
@blurrah
Copy link
Contributor

blurrah commented Sep 26, 2018

I have gone ahead and merged your PR for removing the restrictive constraint and will try to get a new release up today as it's been long overdue.

Removing Wagtail from the constraints sounds like a good plan though I'm always a bit wary about the ease of rollbacking the installation if it's not working properly. In this case I definitely know that wagtail <2 doesn't work with wagtail-personalisation.

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

No branches or pull requests

3 participants