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

PHP lookup constraints #18715

Closed
rarkins opened this issue Nov 2, 2022 · 33 comments · Fixed by #20842
Closed

PHP lookup constraints #18715

rarkins opened this issue Nov 2, 2022 · 33 comments · Fixed by #20842
Labels
auto:reproduction A minimal reproduction is necessary to proceed datasource:packagist priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:requirements Full requirements are not yet known, so implementation should not be started type:feature Feature (new functionality) versioning:composer Composer's semver versioning

Comments

@rarkins
Copy link
Collaborator

rarkins commented Nov 2, 2022

What would you like Renovate to be able to do?

Support php constraints in composer dependency lookups. i.e. do not propose an update if it is incompatible with the current php constraint.

For example, with these constraints: https://github.com/laminas/laminas-diagnostics/blob/df6b5e8d0874f7317b834901c1f6a137ce42ffee/composer.json#L20-L24

Do not propose this change: https://github.com/laminas/laminas-diagnostics/pull/54/files

Because (copy pasted from composer output):

Root composer.json requires laminas/laminas-loader ^2.9.0 -> satisfiable by laminas/laminas-loader[2.9.0].
    - laminas/laminas-loader 2.9.0 requires php ~8.0.0 || ~8.1.0 || ~8.2.0 -> your php version (7.4.99; overridden via config.platform, actual: 7.4.32) does not satisfy that requirement.

Ref: https://packagist.org/packages/laminas/laminas-loader
Ref: https://github.com/laminas/laminas-loader/blob/51ed9c3fa42d1098a9997571730c0cbf42d078d3/composer.json#L24-L26

Note: this seems different to how libraries in Node.js do it, because in Node if you removed support for a major version of the language (e.g. PHP 7) then you should issue a breaking change of your library. The use of e.g. 7.4.99 is also a bit of a composer quirk too.

If you have any ideas on how this should be implemented, please tell us here.

Related discussion with @Ocramius and @internalsystemerror here: laminas/.github#41

So here we have the case where the downstream repo supports 7.4.99 while the new version of the lib requires ~8.0.0 || ~8.1.0 || ~8.2.0.

Obviously to the human eye these are incompatible, but we need a foolproof way for the bot to determine it, because the "Venn diagrams" will not always be so clear. For example if the downstream repo supported 7.4.99 || 8.0.99 then would this be OK or not OK? (in my opinion: not ok).

Would this work?

  • Find all versions which the downstream repo supports (in this simplified case it's only ["7.4.99", "8.0.99"]
  • Find all versions which the upgrading lib version supports (in this case it's essentially every version of 8.x)
  • Reject the upgrade if the second set does not contain every member of the first set. i.e. the lib must support a superset of the downstream consumer in order to be compatible

BTW this could unfortunately let us end up in "stalemates" quite easily where no dependency upgrades are proposed due to the downstream repo having too old or too narrow php requirements, something we should be aware of.

Ideally needs a simplified reproduction because the referenced repo seems quite complicated and therefore difficult to debug.

Is this a feature you are interested in implementing yourself?

Maybe

@rarkins rarkins added type:feature Feature (new functionality) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others auto:reproduction A minimal reproduction is necessary to proceed status:requirements Full requirements are not yet known, so implementation should not be started versioning:composer Composer's semver versioning labels Nov 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Hi there,

Get your issue fixed faster by creating a minimal reproduction. This means a repository dedicated to reproducing this issue with the minimal dependencies and config possible.

Before we start working on your issue we need to know exactly what's causing the current behavior. A minimal reproduction helps us with this.

To get started, please read our guide on creating a minimal reproduction.

We may close the issue if you, or someone else, haven't created a minimal reproduction within two weeks. If you need more time, or are stuck, please ask for help or more time in a comment.

Good luck,

The Renovate team

@Ocramius
Copy link

Ocramius commented Nov 2, 2022

in Node if you removed support for a major version of the language (e.g. PHP 7) then you should issue a breaking change of your library.

This seems like a NodeJS ecosystem detail, but in general, SemVer allows changing dependency ranges as long as the exposed API (declared or inherited from dependencies) doesn't change in breaking ways.

You could say that "the NodeJS ecosystem does it this way", but nothing prevents bumping the dependency to npm or node in a minor release.

Obviously to the human eye these are incompatible, but we need a foolproof way for the bot to determine it

We have 2 problems to solve:

  1. Allow upgrades to "what is compatible" according to SAT rules. SAT is large and complex, so I endorse relying on ecosystem -specific tooling, and not rolling your own.
  2. Detect possible incompatible upgrades (out of SAT rules): this is solved by the current bump strategy

Would this work?

The "would it work?" section above is overly simplistic, and seems to consider the php dependency as "special": in practice, all dependencies need to be considered, php just being part of the whole recipe.

The SAT mechanism inside composer can give you an idea of "what is upgradable" with something like composer update --dry-run and composer outdated, where each dependency probably needs an individual pass. Perhaps it is possible to relax composer.json ranges in a temporary file, before attempting an upgrade.

I wouldn't recommend rewriting that outside composer, since it's one of those problems with exponential calculation complexity.

Ideally needs a simplified reproduction because the referenced repo seems quite complicated and therefore difficult to debug.

Perhaps we can come up with some upgrade scenarios with current composer.json and wished composer.json?

@rarkins
Copy link
Collaborator Author

rarkins commented Nov 2, 2022

  1. Let's set aside the "Should the PHP community release a major version when they change platform support" topic, or move it elsewhere. What matters most anyway is what people do, not what they should do
  2. I think that language/platform compatibility are relative simple things we should do within Renovate. Yes, there may be other SAT-related problems and edge cases we can't and shouldn't solve, but the value of php is fundamentally more important than any of the others
  3. Using composer during the lookup stage to do a full SAT resolve is unlikely to be feasible. It could not be optimised due to everyone's unique dependency tree, so would need to be run for every repo and potentially add minutes to each lookup. We don't use any third party resolvers during lookup due to painful experience and won't change that decision lightly
  4. If libraries do wrong things (e.g. claim certain php compatibility but their dependency tree doesn't reflect that, so composer install fails) then it's not Renovate's job to magically work around those. In fact, it's good if Renovate can attempt to upgrade them and fail, because then the merge confidence value can one day be used to feed upstream to developers that they have a problem.
  5. Related to the above, it is ok for Renovate to sometimes raise PRs which fail. i.e. an "Artifacts error" doesn't itself mean Renovate has done something wrong. Sometimes users should know that a new version is failing
  6. Because we don't want to be too opinionated, we should allow users to configure whether they like (4) or not. For example we could have an option to suppress upgrades whenever the composer lock file update fails to resolve, or fails to resolve for certain reasons
  7. We can maybe try to "roll back a release and try composer again" in such scenarios.. but it could be complicated and time consuming. To me this is a nice to have for last step in future

Now although I see your opinion is firmly about using composer, if I look at the laminas repo with all the wrong upgrades you referenced, wouldn't nearly all of them have been prevented with the simple php constraints resolution step I proposed?

@Ocramius
Copy link

Ocramius commented Nov 11, 2022

Sorry, I stashed the discussion on the side for a while :-)

Answering questions from bullet points above

What matters most anyway is what people do, not what they should do

This makes sense inside @renovatebot: you build a product, and you support the current use-cases, and I absolutely understand that. As library maintainers, we try to pioneer the way things should be done: some approaches will stick, others won't.

I have a good track record of both successes and failures there, but please imagine this discussion as an attempt to improve the situation (or failing at doing so), rather than following the herd :)

the value of php is fundamentally more important than any of the others

Disagree: php is just one of the many dependencies in the graph, it just happens that "platform" dependencies could be fixed at a value that depends on the environment, most of the time.
For renovate as a product, it certainly is of more importance: renovate should certainly endorse PHP upgrades (and it does, since recently 💪 )

Using composer during the lookup stage to do a full SAT resolve is unlikely to be feasible.

Understandable limitation, and using full SAT would probably make renovate way too expensive to run anyway.

If libraries do wrong things (e.g. claim certain php compatibility but their dependency tree doesn't reflect that, so composer install fails) then it's not Renovate's job to magically work around those.

To clarify the scope of the initial discussion, the problem was the opposite: libraries had stricter dependency ranges, and "bumping" to then caused incompatibilities.

In fact, it's good if Renovate can attempt to upgrade them and fail

This works for applications, which are leafs in a dependency graph, but for libraries, where downstream BC is important, incompatible upgrades become very noisy, compared to the healthy signal of "this is what you can upgrade to".
Sometimes, a major release needs to be held back for years, and having a process that constantly bumps to incompatible ranges can become a problem.

For example we could have an option to suppress upgrades whenever the composer lock file update fails to resolve, or fails to resolve for certain reasons

This is certainly an option: having such upgrades reported in the "dependency dashboard" would be super-neat, for example. I think lock file upgrades are still managed via SAT and ecosystem-specific dependency ranges though: won't that strain your infrastructure?

We can maybe try to "roll back a release and try composer again" in such scenarios.. but it could be complicated and time consuming. To me this is a nice to have for last step in future

Un-doing diffs is complex: probably not a viable approach.

wouldn't nearly all of them have been prevented with the simple php constraints resolution step I proposed?

For the one day where we introduced "bump": possibly. Most upgrade issues were caused by transitive dependencies, and the current issue is certainly PHP 8.2, which is set to release this month.
"php" is just the biggest/noisiest scenario though: please refer to the generalized example that I wrote below, for more clarity :-)

A more practical example of the OP issue

As an example, take these 3 dependencies.

{
    "name": "my/project",
    "require": {
        "dependency/a": "^1",
        "dependency/b": "^1"
    }
}
{
    "name": "dependency/a"
    "version": "1"
    "require": {
        "dependency/b": "^1"
    }
}
{
    "name": "dependency/b"
    "version": "1"
}

With following change in the ecosystem:

{
    "name": "dependency/a"
-    "version": "1"
-    "version": "2"
-    "require": {
-        "dependency/b": "^1"
-    }
+    "conflict": {
+        "dependency/b": "*"
+    }
}
{
    "name": "dependency/b"
-    "version": "1"
+    "version": "1.1"
}

The issue that we had with renovate so far (which sparked the entire discussion) is that renovate will propose this impossible upgrade with the "bump" approach:

{
    "name": "my/project",
    "require": {
-        "dependency/a": "^1",
+        "dependency/a": "^2",
-        "dependency/b": "^1"
+        "dependency/b": "^1.1"
    }
}

Doing this consciously with the "bump" is OK: we don't need to change existing mechanisms, and users that want to be notified about this problem should keep using that.
What we (@laminas) were wondering, is whether we could have a different strategy that holds off some of these upgrades, and proposes only the feasible ones:

{
    "name": "my/project",
    "require": {
        "dependency/a": "^1",
-        "dependency/b": "^1"
+        "dependency/b": "^1.1"
    }
}

With this approach, we could apply specific labels to "bump", if we still want to use that, whilst auto-merging the compatible ones.

@Ocramius
Copy link

Meanwhile, experienced the same problematic behavior in an old project (closed source) that is slowly being upgraded with a self-hosted renovatebot.

Specifically, the project is stuck on "dependency/a": "^1", and renovate tries to upgrade to "dependency/a": "^9" in one shot.

What we'd really need is for it to attempt feasible upgrades only, so "dependency/a": "^5" (for example, in our case).

@viceice
Copy link
Member

viceice commented Nov 16, 2022

Meanwhile, experienced the same problematic behavior in an old project (closed source) that is slowly being upgraded with a self-hosted renovatebot.

Specifically, the project is stuck on "dependency/a": "^1", and renovate tries to upgrade to "dependency/a": "^9" in one shot.

What we'd really need is for it to attempt feasible upgrades only, so "dependency/a": "^5" (for example, in our case).

you can use separate multiple major config option as workaround, so you get a PR for each major version. use dashboard approval to update one by one. and don't overload your CI.

@Ocramius
Copy link

@viceice as in create specialized configs for individual dependencies? There are hundreds :D

@viceice
Copy link
Member

viceice commented Nov 16, 2022

no, for start i would set approval for all packages

@back-2-95
Copy link

After official PHP 8.2 release we now get this for most of our PRs created by Renovate.

We usually have the php constraint in our projects e.g. "php": "^8.1". But now Renovate I guess "runs" the Composer with PHP 8.2? Is there a way to define running PHP version for Renovate?

@internalsystemerror
Copy link
Contributor

@back-2-95 that's a slightly different issue in that ^8.1 includes 8.2 so that's what's being used. You can override this in a couple of ways which come to mind:

@WyriHaximus
Copy link

@rarkins As promised on Twitter, here is a reproduce repository: https://github.com/WyriHaximus/renovate-ignore-php-version

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 15, 2023

@WyriHaximus thanks! I would like to make sure I understand the reproduction.

I see one PR, updating thecodingmachine/safe to v2. According to Packagist, v2 of this package requires php: ^8.0, meanwhile the latest 1.x I find requires php: >=7.2.

Looking at the reproduction project itself, it requires "php": "^8 || ^7.4.7",. Therefore upgrading to v2 would mean you can no longer support ^7.4.7.

Do I understand correctly?

@herndlm
Copy link
Contributor

herndlm commented Jan 15, 2023

@rarkins I also had a quick thought about this yesterday. What do you think of "simply" filtering out packages with an incompatible php version on lookup? That would be #2355 I suppose. It's obviously not a generic fix for this issue here, but I think it might bring us one step forward and get rid of the most common annoyance? Especially since now platform upgrades are possible as well.

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 15, 2023

Yes, that's what I'm working on right now. But I encountered a hitch. If you look at https://packagist.org/p2/thecodingmachine/safe.json for example, only the first (latest) version includes a requires section. Older versions like 2.3.0 seem to be missing them. If Packagist doesn't tell us the requires.php for each version then we can't filter. Any ideas?

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 15, 2023

Meanwhile the deprecated v1 API for Packagist does include require, e.g. https://packagist.org/p/thecodingmachine/safe.json

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 15, 2023

So for now this issue is blocked until something like the following happens:

  • Someone figures out how to get require metadata from the v2 API
  • Someone from Packagist adds this metadata to the API
  • We decide we should use the deprecated v1 API

I see that there are 8 occurrences of the "require": string in the p2 response. Wondering if this is a "minification" feature and it's only printed any time it changes.

https://github.com/composer/metadata-minifier

@herndlm
Copy link
Contributor

herndlm commented Jan 15, 2023

a php require is totally optional AFAIK, so in case it's not there you can assume the lib states that it works with any php version. also most likely we need something like the reverse logic if there is a php conflicts or so
UPDATE: sorry just later realised that you were talking mostly about an API version difference there and the fact that only the latest has it for v2..

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 15, 2023

I have a working draft for this in #19851

First, I'd really like some more advanced reproductions. For the one by @WyriHaximus, it now suppresses the v2 upgrade, but it would be good if we also have an example where there are multiple possible upgrades, some of which have incompatible updates and some of which are compatible, and it picks the highest compatible.

Second, I would like to put this behind a config option (i.e. opt in) so that we can push this to production faster without needing to think/test about all the possible ways this could go wrong. For example, what if the current version is already incompatible?

@WyriHaximus
Copy link

@rarkins thanks, responding in line 👍

@WyriHaximus thanks! I would like to make sure I understand the reproduction.

I see one PR, updating thecodingmachine/safe to v2. According to Packagist, v2 of this package requires php: ^8.0, meanwhile the latest 1.x I find requires php: >=7.2.

Looking at the reproduction project itself, it requires "php": "^8 || ^7.4.7",. Therefore upgrading to v2 would mean you can no longer support ^7.4.7.

Do I understand correctly?

Yes, but widening to ^2 || ^1 is somewhat what I expect from auto. But my expectations might be wrong here, and maybe out of scope.

I have a working draft for this in #19851

Sweet, will have a look!

First, I'd really like some more advanced reproductions. For the one by @WyriHaximus, it now suppresses the v2 upgrade, but it would be good if we also have an example where there are multiple possible upgrades, some of which have incompatible updates and some of which are compatible, and it picks the highest compatible.

Sure, I can give you a repo for that situation as well.

Second, I would like to put this behind a config option (i.e. opt in) so that we can push this to production faster without needing to think/test about all the possible ways this could go wrong. For example, what if the current version is already incompatible?

Works for me. Currently looking at having to do several 100 PR's by hand because of (at least) two situations so if I can help test this and get it done sanely with Renovate I'm in!

@WyriHaximus
Copy link

WyriHaximus commented Jan 15, 2023

First, I'd really like some more advanced reproductions. For the one by @WyriHaximus, it now suppresses the v2 upgrade, but it would be good if we also have an example where there are multiple possible upgrades, some of which have incompatible updates and some of which are compatible, and it picks the highest compatible.

@rarkins Hope this is what you meant:

Set up another repo where the required package jumps from v4.x to v6: https://github.com/WyriHaximus/renovate-php-multiple-updates-possible

v4: ^8 || ^7.4
v5: ^8
v6: ^8.2
(From: https://packagist.org/packages/wyrihaximus/async-test-utilities)

Expectation auto widens it from ^4.2 to ^6 || ^5 || ^4.2, but again my expectation might be wrong for auto here.

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 15, 2023

Thanks. FYI rangeStrategy=replace is the default. You can switch it to rangeStrategy=widen for most managers (I think including composer). This is a separate/tangential topic to compatibility and it's better not to complicate this issue with it.

@WyriHaximus
Copy link

Thanks.

Please ping me if you require more and/or more complicated situations. II'll try to set up a very wide-ranged complex one with multiple packages tonight and see how it responds to that.

FYI rangeStrategy=replace is the default. You can switch it to rangeStrategy=widen for most managers (I think including composer). This is a separate/tangential topic to compatibility and it's better not to complicate this issue with it.

Do you prefer that with the reproduction repositories as well? And yes that is a whole different feature request with the things I would like to do there. And yes composer supports widen but that might not always be the thing I'd prefer. But again different scope :).

@herndlm
Copy link
Contributor

herndlm commented Jan 16, 2023

@rarkins Regarding the missing info in the v2 API (sorry I misunderstood that in your last comment about it initially):
You were right. This is indeed minified and uses https://github.com/composer/metadata-minifier for expansion as you assumed. See also https://packagist.org/apidoc#get-package-data where this is documented.

What's the best way forward then? It looks like the minification package hasn't been changed for almost 2 years and the v2 API response is referencing it with a version ("minified":"composer/2.0"). I assume this has to be stable within major composer / API releases. We could re-implement the same logic here I suppose? Might still be better than using the deprecated v1 API.. Looking at https://github.com/composer/metadata-minifier/blob/1.0.0/src/MetadataMinifier.php#L16-L46 the expansion part isn't that hard to duplicate actually. I can look at this too.

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 16, 2023

We can use v2 API and support de-minification

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 16, 2023

(already done, actually)

@herndlm
Copy link
Contributor

herndlm commented Jan 16, 2023

nice! :)

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 16, 2023

Set up another repo where the required package jumps from v4.x to v6: WyriHaximus/renovate-php-multiple-updates-possible

v4: ^8 || ^7.4 v5: ^8 v6: ^8.2 (From: packagist.org/packages/wyrihaximus/async-test-utilities)

Expectation auto widens it from ^4.2 to ^6 || ^5 || ^4.2, but again my expectation might be wrong for auto here.

I have a different expectation here, so we should clarify. If you update wyrihaximus/async-test-utilities to v5 or v6 it means that it no longer supports PHP v7, and therefore the parent library now longer supports it too.

On the other hand, your composer.json specifies "php": "7.4.7" so that would fail if you try to install v5 or v6 of the dependency.

So my expectation is: while your library needs to support v7, you cannot have a dependency which supports only v8, and such updates should be suppressed until you drop support for v7 too.

On the other hand, if Renovate widens to support v5 and v6 as you suggest.. can't that be done today if you set rangeStrategy=widen? Is it your expectation to support newer versions as long as there's at least one php version overlapping?

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 16, 2023

Using latest/production Renovate only, and the reproduction repo with adjusted config:

In other words, it appears like this desired use case is already satisfied through config only (mainly, rangeStrategy=widen). These PRs were generated using standard PR and not my WIP feature to support filtering out updates which are incompatible.

@WyriHaximus
Copy link

@rarkins Thanks for the update, just finished writing down my thoughts on the rangeStrategy in #19866

On the other hand, your composer.json specifies "php": "7.4.7" so that would fail if you try to install v5 or v6 of the dependency.

Correct, however when someone uses a package with that defined in there that value is ignored.

On the other hand, if Renovate widens to support v5 and v6 as you suggest.. can't that be done today if you set rangeStrategy=widen? Is it your expectation to support newer versions as long as there's at least one php version overlapping?

Yes. And I think I came up with a way to achieve that #19866 that doesn't require massive and complex changes.

@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 35.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Ocramius
Copy link

Ooooh, thanks @Lctrs! I suppose this doesn't yet use SAT, but some sort of advanced guesswork, right?

@rarkins
Copy link
Collaborator Author

rarkins commented Mar 15, 2023

It's based on the language ranges of the direct dependencies. It doesn't identify/correct if dependencies wrongly declare their language support (e.g. they say they support v1 and v2 of a language, but they themselves use a dependency which supports only v2).

Also note it's opt-in, and that this version is not yet live in the github app

@Ocramius
Copy link

Good to know: doesn't match my needs exactly, but I fully understand all limitations previously discussed 👍

Thanks for working on this, it already helps tons!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto:reproduction A minimal reproduction is necessary to proceed datasource:packagist priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:requirements Full requirements are not yet known, so implementation should not be started type:feature Feature (new functionality) versioning:composer Composer's semver versioning
Projects
None yet
8 participants