search

laminas / .github

Laminas Organization Templates
https://getlaminas.org/
5 stars 37 forks source link

`require-dev` upgrades don't seem to respect `composer.json` constraints #41

Closed Ocramius closed 1 year ago

Ocramius commented 1 year ago

Bug Report

require-dev upgrades don't seem to respect composer.json's config.platform.php definition, nor the existing dependency ranges.

Probably caused by #39.

This affects following patches (big list):

Ocramius commented 1 year ago

My initial suspicion is that the bump operation (in Renovate) is buggy, and doesn't use composer to perform the bump (therefore not looking at the dependency graph at all).

This is highlighted by the fact that we have similar issues with packages that declare {"config": {"platform": {"php": "8.0.99"}}}, as well as packages that only declare {"require": {"php": "~7.4.0 || ~8.0.0"}}, for example.

My suspicion is that bump picks whatever is the currentVersion according to renovate, and just puts it in the dependency range as-is, then uses composer to update composer.lock (and fails).

If that is the case, we can either:

  1. revert the change
  2. report an upstream issue
internalsystemerror commented 1 year ago

It doesn't use composer to perform the bump by design, and the same applies with any proposed updates except for the lock file, which always uses composer. Bump will always try to set the latest available version, whereas replace will only set the version if it falls outside of the current constraint.

The idea is that renovate is telling you about an available update that is currently failing for whatever reason. The config.platform.php field is still being used for all of these and is only used for regenerating the lock file when composer.json changes at all. This is why we have renovate adding a comment telling us that the composer update failed, because we're using the PHP version in config.platform.php.

The same is true for other platforms, npm, yarn etc. So all of the issues linked are telling us that there are new versions available, but for whatever reason they can't be used.

Ocramius commented 1 year ago

Ok, but then that is our misunderstanding, and the bump strategy won't fly at all :-)

What's the best strategy that does a "pick the latest compatible version, and restrict to that range"?

rarkins commented 1 year ago

Why choosing bump to begin with, btw? If you only want to support the latest version then pin versions. Otherwise retain ranges

Ocramius commented 1 year ago

Why choosing bump to begin with, btw?

Only affects require-dev, in this case, not the dependency ranges exposed to downstream users.

If you only want to support the latest version then pin versions. Otherwise retain ranges

Pinning completely means not moving forward at all: the idea is to keep chopping away at the tail of older versions, while still allowing newer releases to be tested (if, for example, our tooling gets stuck, or a user is running tests on an oldstable release while bisecting a bug).

EDIT: adding a practical example of why this is being done.

This is usual business with require-dev tooling: it's much less stable than we'd wish, so restricting the --prefer-lowest range as much as possible is a good idea, while still allowing discovery of breakages in newer upgrades.

I'd also add that we can perhaps drop latest checks, assuming our composer.lock files are updated daily.

rarkins commented 1 year ago

Can we step back and you describe why you use ranges instead of exact versions (Eg it's a library) and what you're hoping to achieve each dependency update?

Ocramius commented 1 year ago

@rarkins I expanded above with an example. For more context, most of laminas/* is libraries.

What we achieve with regular updates:

  1. discovery of ecosystem-wide regressions
  2. discovery of regressions inside our own libraries (they depend on each other, and we sometimes cause trouble to ourselves)
  3. encouraging upgrades for end users (want the new feature? here's a carrot on a stick: upgrade your other dependencies too!). Note that we don't bump require dependencies automatically, but we rather keep chopping away at supported versions over time, as a conscious decision.
  4. expanding compatibility to newer dependencies consciously (we don't support what we didn't test)
internalsystemerror commented 1 year ago

FWIW I agree with @rarkins here, I don't think we need to bump dependency versions and can pin them BECAUSE these aren't used downstream.

For clarification, the difference between pinning the dependencies and bumping them will mean:

Updates will be proposed the same with both, the difference is only is whether the version number has a constraint in front of it. So we would be notified still when certain version numbers fail, and lowest vs latest vs --prefer-lowest would be for require dependencies only.

Ocramius commented 1 year ago

Careful with pinning: it doesn't work within CI matrix configurations with multiple PHP versions.

In a CI configuration with:

We run:

The locked (pinned) dependencies will unlikely work on PHP 7.4, 8.0 and 8.1 at the same time, and that has pretty much always been that way within the PHP ecosystem. That's why they are not part of our matrix runs.

That's why there should be a range, where the range starts with whatever is supported on 7.4 (in the example above).

EDIT: The same applies with CI matrix configurations where we test things like [mysql, postgresql, mssql, oracledb]: each will come with their own ext-<something> little corner of hell :D

Ocramius commented 1 year ago

I'd like to throw the question from my previous comment in the room again:

What's the best strategy that does a "pick the latest compatible version, and restrict to that range"?

If the answer is that there isn't such an upgrade strategy, rolling back #39 is good enough for me.

rarkins commented 1 year ago

Thanks. I may need until tomorrow to get back to you though as I'm lacking remaining time in today.

As a general rule we have erred on the side of "propose upgrades and communicate the breakage" so that people are aware of how out of date they are based on constraints. However we have for python already implemented the ability to filter upgrades based on compatibility. The key thing is that you have the capability (and visibility) to know you're out of date with the language constraints (Eg php) so that you don't end up in state where you're really out of date but unaware of it (no upgrades!)

internalsystemerror commented 1 year ago

I've been sat here considering the options and renovate is set up around informing you that an update is available allowing you to address it.

From the rangeStrategy documentation:

Behavior: auto = Renovate decides (this will be done on a manager-by-manager basis) pin = convert ranges to exact versions, e.g. ^1.0.0 -> 1.1.0 bump = e.g. bump the range even if the new version satisfies the existing range, e.g. ^1.0.0 -> ^1.1.0 replace = Replace the range with a newer one if the new version falls outside it, and update nothing otherwise widen = Widen the range with newer one, e.g. ^1.0.0 -> ^1.0.0 || ^2.0.0 update-lockfile = Update the lock file when in-range updates are available, otherwise replace for updates out of range. Works for bundler, composer, npm, yarn, terraform and poetry so far in-range-only = Update the lock file when in-range updates are available, ignore package file updates

The in-range-only strategy may be useful if you want to leave the package file unchanged and only do update-lockfile within the existing range. The in-range-only strategy behaves like update-lockfile, but discards any updates where the new version of the dependency is not equal to the current version. We recommend you avoid using the in-range-only strategy unless you strictly need it. Using the in-range-only strategy may result in you being multiple releases behind without knowing it.

It doesn't really do any compatibility checks and I understand why this is not default as you could get into the position (esp when PHP was not supported) where there are updates, but because you're stuck on older versions it wouldn't tell you about it. At least this way it will tell you there is an update, and also that it isn't compatible with the current PHP version you're running.

It could be added as a feature, another strategy bump-compatible which checks that the new version is compatible with the PHP range (if specified), and does nothing otherwise. There may be updates available that we're not told about, but since the php package is now supported as an update, it may be feasible to implement now.

Looking at the in-range-only and update-lockfile options, we could use one of those, but then we would still get failing PRs if the lock file is generated on 7.4, and the new package requires >=8.0.

Ocramius commented 1 year ago

The key thing is that you have the capability (and visibility) to know you're out of date with the language constraints (Eg php) so that you don't end up in state where you're really out of date but unaware of it (no upgrades!)

Makes sense, yes.

TBH, we are usually rarely in that state, since the need for upgrades is usually flagged by end-users, if everything gets stuck (takes very little time, users are very eager to try alpha/beta software too).

However we have for python already implemented the ability to filter upgrades based on compatibility.

Ah, that would be what could work for PHP too, but I suppose we'd roll back for now. We can also try contributing upstream, since it benefits everyone :+1:

@internalsystemerror I think you did a lot of investigation that got us the same results of what @rarkins told us a few moments before: WDYT about a revert of #39, for now?

internalsystemerror commented 1 year ago

Revert works for me 👍. If I can contribute towards this on renovatebot/renovate I will!

Ocramius commented 1 year ago

OK, rolling back #39 (via UI: I'm lazy)

rarkins commented 1 year ago

Can we move the discussion to https://github.com/renovatebot/renovate/issues/18715 ?