-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
gh-111062: Add nogil build test on GitHub Actions #111060
Conversation
corona10
commented
Oct 19, 2023
•
edited
Loading
edited
- Issue: Add GitHub Action for checking free-threaded build #111062
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.
IMO NoGIL is too experimental to hold merging PRs as this point. I would prefer to rely on buildbots for now.
cc @colesbury
When you're done making the requested changes, leave the comment: |
What's the rationale for adding a pre-commit CI? This PR is related to an issue which is closed. |
Well, In my memory, sub interpreter people suffered from the catch-up broken build. Then what about adding CI that allows failure? (It's possible) |
Well, I just think that it will be great if we can check up the issue earlier in the PR round.
That's why I am just adding checking build status not including test phase. |
When I added a FreeBSD CI, people get annoyed by CI failures even if the CI was non-voting since FreeBSD is only a "Tier-3" buildbot. |
Did it allow pass status even if the CI itself failed? |
People complained that the whole PR was marked as "failed" when the FreeBSD CI, whereas the FreeBSD CI was not mandatory. |
In Github Action we can ignore the "failed" status as a false positive result. We did the same thing in pyperformance project. |
I think a GitHub action would be helpful, but it's not critical. If there is opposition to adding it, then it can wait. |
If people don't like seeing an (occasional?) red result, even if non-voting (i.e. doesn't block merge), another option is to append something like The utility of this is debatable, as you have to click through to check if it really passed or failed. |
I agree with this. I don't see the value of signal that always passes and requires a human to dig in to check for the real result. |
We're probably best off waiting for the official SC acceptance so we know more about how this should be tested. Hopefully it'll be ready soon. (And if something like this PR, we should do some refactoring of build.yml, it's getting pretty big. See #110794 (comment).) |