Force usage of latest patch version to avoid JIT bugs #11270
Conversation
danog
added
the
release:fix
|
Isn't that a bit harsh? If JIT is the problem, then it could be checked for that in combination with the version. Also if psalm is hard-coded to require a specific php version it should say so in its composer.json. Otherwise composer will happily download a version that can't be used. |
|
That is kind of the point, hardcoding a composer.json version will prevent users from getting newer versions of psalm, but a runtime check will load it just fine. The ondrej ppa repos for Ubuntu have the latest 8.3.16 with all the fixes; I do not feel comfortable with slowing down Psalm due to some distros lagging behind in patch releases. |
|
But letting people install psalm despite knowing it will not run is just bad dx. I just ran into it and was confused why i even could install the version if it's not runnning with it... IMHO bumping the min version in composer.json is here the way to enforce newer php versions. |
It shouldn't be an exact version, just a minimum version. For example: "php": "~8.1.31 || ~8.2.27 || ~8.3.16 || ~8.4.3" |
|
Either way, specifying a more strict min version will silently prevent composer update from loading the latest version. For a development tool like Psalm, I consider a runtime check to be more than acceptable. |
| foreach ([ | ||
| 8_01_31 => '8.1.31', | ||
| 8_02_27 => '8.2.27', | ||
| 8_03_16 => '8.3.16', | ||
| 8_04_03 => '8.4.3', | ||
| ] as $version => $txt) { | ||
| $version_m = $version - ($version % 100); | ||
| if ($version_m === $major_minor) { | ||
| if (PHP_VERSION_ID < $version) { | ||
| $issues[] = 'Psalm requires a PHP version ">= ' . $txt . '".' | ||
| . ' You are running ' . PHP_VERSION . '.'; | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
Please just declare composer.json "conflict" ranges next time: this has caused me quite a few headaches, yesterday.
I'd rather get an older psalm version, than one that crashes at startup.
There was a problem hiding this comment.
Thing is, the version bump was made for a specific reason, to avoid crashes and other subtle bugs that would have caused more lost time in pointless debugging than a simple PHP version upgrade...
There was a problem hiding this comment.
From a consumer perspective, a conflict range would've made this clear, and could've been relaxed until a bugfix is applied (if that would ever allow relaxing the dependency range - it's fine to have the limit in place from here on).
A runtime crash is the worst of both worlds:
- it doesn't work
- I don't know it until I run it
Early signaling here is composer update failing to get the latest release.
The current effective result I have right now is me + other people I collaborate with having to manually go in composer.json and pinning vimeo/psalm to an older release.
In fact, I currently use vimeo/psalm:6.4.0 (rollback) on all my systems, and added vimeo/psalm:6.5.0 as an explicit conflict for my colleagues.
There was a problem hiding this comment.
The problem with conflict ranges is that composer does not make them at all clear to users of a package; the dependency resolver simply works around them to fit the current version constraints, regardless of the side effects (bugs in older PHP versions).
This is not the only runtime check, and some requirements (like the VM overcommit requirements) cannot even be expressed using composer.
I will not be adding a dependency constraint to composer.json, because as I said, Psalm is a development tool, and the development environment is not as rigid as a production environment (to a certain extent, i.e. minor bumps will still be expressed using composer, as they are somewhat related to the production environment).
There was a problem hiding this comment.
the dependency resolver simply works around them to fit the current version constraints, regardless of the side effects (bugs in older PHP versions).
We've been working this way for well over a decade now: not getting "latest" is fine, and composer why-not tells you so.
I've even built an entire package around this: https://github.com/Roave/SecurityAdvisories/ - it's designed to exclude dependencies that are a hazard, and people do use it, consciously.
Runtime checks are 100% not the way, and amplify noise around this.
I (relatively experienced PHP dev) for sure wasted a lot of time yesterday dealing with this while trying to work on psalm/psalm-plugin-phpunit#149 - had to roll back vimeo/psalm tons of times while trying out dependency ranges.
What moving this to a runtime check achieved here is:
- confusion (people are putting feedback here for a reason: nobody wants to piss you off, rest assured of that :-P )
- almost all
composer.lockupgrade pipelines ( @renovate / @dependabot ) will fail due to an invalid upgrade - time wasted during development
What should be done next?
The correct way forward here is:
- revert patch
- tag new patch release (so that upgrades allow jumping to a working release again, including automated upgrades performed so far)
- re-apply patch
- add dependency range in
"conflict" - tag new patch release
Note that deleting a release is non-viable, unless it's a security risk.
"it's a development tool"
As for the "it's a development tool": it does not matter if it is - it's part of the dependency graph, and the dependency graph is designed to say "these work together" or "these don't work together".
In addition to that, even if the dependency graph is split between --dev and --no-dev dependencies, the "these work together" should still apply for the --dev part.
There was a problem hiding this comment.
Marco, I value your input, and look forward to collaborating with you (will be taking a look at your PRs next week).
I am not too willing to die on this hill: in general, it's not about this specific change, it's about the possiblity for me as the maintainer of Psalm to direct users to make changes to their environment, to avoid them footguns when working with Psalm.
In my long time as a PHP engineer, I have always tried to proactively prevent bugs from even reaching users, guaranteeing the best possible experience when actually using my tools, be it using warnings to update to the last version on startup (like in MadelineProto, will probably implement this in Psalm as well, given that phpstan has them too), or using runtime exceptions for particularly bad platform bugs (i.e. did something similar for MadelineProto for a recent fiber-related bug, and @trowski did the same inside of amphp itself as well), or even automatic static analysis on code written by the users, to proactively detect possible issues.
I do not want users to waste time debugging weird exceptions and crashes caused by PHP bugs (like recently, I already got two new issues caused by a PHP bug on older versions of PHP, combined with the new amphp-based concurrency model).
A (patch release, not even a major/minor!) PHP version upgrade is a minor inconvenience, bypassable using any modern PHP distribution method (ondrej ppas on debian/ubuntu), and does not require any additional refactoring work on the codebase on behalf of users.
This is also the case for other runtime checks as well (such as #11218).
I am a user and great fan of your security-advisories project, because it allows library developers to bypass composer's long-time limitations, to prevent users from getting vulnerable/broken versions of libraries.
I am trying to apply the same rationale behind security-advisories to Psalm as well, using the only way I have available to me, given that we're talking about PHP platform bugs, and not issues with userland libraries.
There was a problem hiding this comment.
I feel like the message didn't come through, but I'm also just making a point, and not really willing to explain further than I already did :D
We should probably talk about it over 🍻 at PHPDay or such :)
There was a problem hiding this comment.
Ayy, now that's a nice idea!
Fixes #11268