Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Oct 19, 2023

Copy link
Member

@vstinner vstinner 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

bedevere-app bot commented Oct 19, 2023

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
Member Author

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
Member Author

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
Member Author

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
Member Author

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

@corona10 corona10 changed the title gh-108223: Add nogil build test on Github Action gh-111062 Add nogil build test on Github Action Oct 19, 2023
@corona10 corona10 changed the title gh-111062 Add nogil build test on Github Action gh-111062: Add nogil build test on Github Action Oct 19, 2023
@corona10 corona10 marked this pull request as ready for review October 19, 2023 09:14
@hugovk hugovk changed the title gh-111062: Add nogil build test on Github Action gh-111062: Add nogil build test on GitHub Actions Oct 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

hugovk commented Oct 19, 2023

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

itamaro commented Oct 19, 2023

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

hugovk commented Oct 20, 2023

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

@corona10 corona10 closed this Oct 29, 2023
Sign up for free to 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