Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
wouterj wants to merge1 commit intosymfony:4.4fromwouterj:php81-experimental

Conversation

@wouterj
Copy link
Member

QA
Branch?4.4
Bug fix?no
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

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).

@nicolas-grekas
Copy link
Member

Please don't. It's green since a few months. Keeping this as required prevent us from merging PRs that break that.

@stof
Copy link
Member

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
Copy link
Member

nicolas-grekas commentedApr 25, 2022
edited
Loading

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
Copy link
Member

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
Copy link
Member

nicolas-grekas commentedApr 25, 2022
edited
Loading

Only occasionally, and that's no different from doctrine failing temporarily. At least until today included to me.
Having 8.2 green was a milestone on its own and made us fix issues earlier.
Making it fail silently will make us drift from that milestone. Unless we have a strong reason to do it (eg 8.2 being red with unreasonable time-to-green.), I'd like to keep open eyes on the topic...

@nicolas-grekas
Copy link
Member

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
Copy link
MemberAuthor

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
Copy link
Member

nicolas-grekas commentedApr 25, 2022
edited
Loading

Failures on appveyor are much more annoying (yet needed) :)

@nicolas-grekas
Copy link
Member

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)

worth a try yes!

@stof
Copy link
Member

using required checks would prevent us from pushing to github without these checks passing, which would affect us in 2 ways:

  • it would prevent us merging when we decided that the failure was a volatile test that failed and we're OK with this
  • it would prevent us from squashing or retargetting locally with the Symfonygh tool, as the result of that local operation would not have the checks on them.
wouterj reacted with confused emoji

Copy link
Member

@chalasrchalasr left a 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.

nicolas-grekas added a commit that referenced this pull requestApr 25, 2022
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
Copy link
Member

nicolas-grekas commentedApr 25, 2022
edited
Loading

Closing as explained. 8.2 is back to green since#46170.
We can reconsider of course when we'll have a real issue with keeping 8.2 green.

@wouterjwouterj deleted the php81-experimental branchApril 26, 2022 09:01
@wouterj
Copy link
MemberAuthor

For reference, sincephp/php-src@f869a54 (merged yesterday) builds are red again:https://github.com/symfony/symfony/runs/6240314891

@nicolas-grekas
Copy link
Member

Perfect, that's exactly what a ci is for :)
Time to rebase the fix I submitted a few days ago to make it green again:#46167

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof approved these changes

@derrabusderrabusderrabus approved these changes

@chalasrchalasrchalasr approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

7 participants

@wouterj@nicolas-grekas@stof@fabpot@derrabus@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp