Skip to content

Conversation

@corona10
Copy link
Member

@corona10corona10 commented Oct 19, 2023

Copy link
Member

@vstinnervstinner left a 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

@bedevere-app
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner
Copy link
Member

What's the rationale for adding a pre-commit CI? This PR is related to an issue which is closed.

@corona10
Copy link
MemberAuthor

corona10 commented Oct 19, 2023

IMO NoGIL is too experimental to hold merging PRs as this point. I would prefer to rely on buildbots for now.

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)

@corona10
Copy link
MemberAuthor

corona10 commented Oct 19, 2023

What's the rationale for adding a pre-commit CI? This PR is related to an issue which is closed.

Well, I just think that it will be great if we can check up the issue earlier in the PR round.
For example, we only able to check #110764 in the local environment or after merge status.

IMO NoGIL is too experimental to hold merging PRs as this point

That's why I am just adding checking build status not including test phase.

@vstinner
Copy link
Member

Then what about adding CI that allows failure? (It's possible)

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.

@corona10
Copy link
MemberAuthor

corona10 commented Oct 19, 2023

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?
I meant that we can add the CI that only check whether the build is successful or not. And if we also need to allow the broken build for other PR, we can do it.

@vstinner
Copy link
Member

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.

@corona10
Copy link
MemberAuthor

corona10 commented Oct 19, 2023

People complained that the whole PR was marked as "failed" when the FreeBSD CI

In Github Action we can ignore the "failed" status as a false positive result. We did the same thing in pyperformance project.
python/pyperformance#274

@corona10corona10 changed the title gh-108223: Add nogil build test on Github Actiongh-111062 Add nogil build test on Github ActionOct 19, 2023
@corona10corona10 changed the title gh-111062 Add nogil build test on Github Actiongh-111062: Add nogil build test on Github ActionOct 19, 2023
@corona10corona10 marked this pull request as ready for review October 19, 2023 09:14
@hugovkhugovk changed the title gh-111062: Add nogil build test on Github Actiongh-111062: Add nogil build test on GitHub ActionsOct 19, 2023
@colesbury
Copy link
Contributor

I think a GitHub action would be helpful, but it's not critical. If there is opposition to adding it, then it can wait.

@hugovk
Copy link
Member

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 || true to the test command to always make it "pass".

The utility of this is debatable, as you have to click through to check if it really passed or failed.

@itamaro
Copy link
Contributor

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.
in that sense, the manual buildbot triggerring seems preferable to me.

@hugovk
Copy link
Member

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@corona10@vstinner@colesbury@hugovk@itamaro