Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Mark PHP 8.2 CI as experimental#46160
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedApr 25, 2022
Please don't. It's green since a few months. Keeping this as required prevent us from merging PRs that break that. |
stof commentedApr 25, 2022
it is not green since a month. It was green during one month, then red since yesterday due to changes done in PHP, which turned all PRs to a red CI while none of them was responsible. |
nicolas-grekas commentedApr 25, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I didn't notice it was red for so long. But that doesn't change my point: I'd like to continue giving this strategy a try. We run all our deps as dev-master for the same reason: it increases the pressure to take care of our dependencies, and this is good pressure. Eg when doctrine deps make our builds green, this forces us to have a conversation with or send PRs to doctrine, and that's a very sane thing to me. Doing the same with PHP 8.2 provides the same benefits to me. |
fabpot commentedApr 25, 2022
I think everybody agrees with what you're saying Nicolas, but at the same time, contributors are not aware of this and they have all their PRs red without being able to fix them. |
nicolas-grekas commentedApr 25, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Only occasionally, and that's no different from doctrine failing temporarily. At least until today included to me. |
nicolas-grekas commentedApr 25, 2022
Sorry I double down on this one: this hides failures that have to be fixed anyway. Clear 👎 on this one today. I'm going to send a PR to fix the build instead. |
wouterj commentedApr 25, 2022
I do think it's useful feedback to see the actual failures (instead of a wrong green check every time). However, I do believe this impacts the DX of contributors. I created this PR after seeing this error in#46157 and I had to investigate whether it was due to my changes. I understand Symfony's build system, so it didn't take me long. But, a normal (or new) contributor is much more worried about this red cross in their PR (I've had to tell people often on Slack to ignore build failures that are unrelated to their changes). If we reject this PR, can we consider using the "required check" setting of GitHub Actions, to better indicate to users not to worry about a failing PHP 8.2 build? (by not marking it required) It's unfortunate that GitHub doesn't come with a better "allow failures" feature currently, but I think we can improve the contributing DX here :) |
nicolas-grekas commentedApr 25, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Failures on appveyor are much more annoying (yet needed) :) |
nicolas-grekas commentedApr 25, 2022
worth a try yes! |
stof commentedApr 25, 2022
using required checks would prevent us from pushing to github without these checks passing, which would affect us in 2 ways:
|
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Getting failed workflow runs is very frustrating and often wastes time/energy. There is no hurry for Symfony to be green on PHP 8.2, we can just take a look at the build from time to time to fix the failures as they come. Less stress for maintainers, less frustration for contributors.
This PR was merged into the 4.4 branch.Discussion----------Fix dumping enums on PHP 8.2| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Followsphp/php-src#8233Makes the CI green again on PHP 8.2.Replaces#46160Commits-------4244aa8 Fix dumping enums on PHP 8.2
nicolas-grekas commentedApr 25, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Closing as explained. 8.2 is back to green since#46170. |
wouterj commentedApr 30, 2022
For reference, sincephp/php-src@f869a54 (merged yesterday) builds are red again:https://github.com/symfony/symfony/runs/6240314891 |
nicolas-grekas commentedApr 30, 2022
Perfect, that's exactly what a ci is for :) |
PHP 8.2 is still in active development (only expected to be released in 7 months time). Imho, we should keep the 8.2 job as experimental, to avoid noisy PR check failures due to upstream changes (e.g. over the weekend, a change in PHP 8.2 caused all new PRs to have a failing check).