8000 gh-111062: Reusable Ubuntu build that supports free-threaded mode as the conditional CI by NCLI · Pull Request #111452 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-111062: Reusable Ubuntu build that supports free-threaded mode as the conditional CI #111452

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 29 commits into from
Oct 30, 2023
Merged

gh-111062: Reusable Ubuntu build that supports free-threaded mode as the conditional CI #111452

merged 29 commits into from
Oct 30, 2023

Conversation

NCLI
Copy link
Contributor
@NCLI NCLI commented Oct 29, 2023 •
edited by bedevere-app bot
Loading

@bedevere-app
Copy link
bedevere-app bot commented Oct 29, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link
bedevere-app bot commented Oct 29, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link
bedevere-app bot commented Oct 29, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link
bedevere-app bot commented Oct 29, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link
bedevere-app bot commented Oct 29, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

corona10
corona10 reviewed Oct 29, 2023
View reviewed changes
.github/workflows/build.yml Outdated
build_ubuntu_free_threaded:
name: 'Ubuntu (free-threaded)'
needs: check_source
if: needs.check_source.outputs.run_tests == 'true'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that the label is attached or not.

corona10
corona10 requested changes Oct 29, 2023
View reviewed changes
Copy link
Member
@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please skip the test if the label is not attached
https://github.com/python/cpython/pull/111452/files#r1375408558

@bedevere-app
Copy link
bedevere-app bot commented Oct 29, 2023

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

corona10
corona10 approved these changes Oct 29, 2023
View reviewed changes
Copy link
Member
@corona10 corona10 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it looks good to me
@hugovk @ezio-melotti Can you please take a look?

@hugovk
Copy link
Member
hugovk commented Oct 29, 2023

I'll add the https://github.com/python/cpython/labels/topic-free-threaded label to check if it triggers.

@hugovk
Copy link
Member
hugovk commented Oct 29, 2023 •
edited
Loading

The label would need to have been added before the workflow runs, which means you need to add it when drafting the PR or do it very quickly.

Or we need to add triggers similar to this:

cpython/.github/workflows/require-pr-label.yml

Lines 3 to 5 in fa35b9e

on:
pull_request:
types: [opened, reopened, labeled, unlabeled, synchronize]

But that adds extra complexity because we don't want to re-trigger all the other CI jobs.

More to the point: do we really need to limit this via a label?

Should we always run it?

After all, we want to make sure there are no regressions for both the GIL and free-threaded builds, whether or not someone considers their PR to somehow related free-threaded.

@corona10
Copy link
Member
corona10 commented Oct 29, 2023 •
edited
Loading

The label would need to have been added before the workflow runs, which means you need to add it when drafting the PR > or do it very quickly.

Or we need to add triggers similar to this:
More to the point: do we really need to limit this via a label?
Should we always run it?

I know this issue, but topiuc-free-thread works are not a big portion of over all PRs so I think that it can be taken into account anyway at this moment. In the near future it can be run anyway once the free-thread version becomes stable but looks like not this moment. :)

And as I commented before, I don't want that free-threaded work to make other people's work stop.

@corona10
Copy link
Member

The label would need to have been added before the workflow runs, which means you need to add it when drafting the PR or do it very quickly.

And it's intended design at this moment, or we need to improve it at later

@corona10 corona10 closed this Oct 29, 2023
@corona10 corona10 reopened this Oct 29, 2023
@corona10
Copy link
Member

And simply re-open the PR will works if the person miss the timing of the labeling

This was referenced Oct 30, 2023
Closed
Closed
hugovk
hugovk approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member
@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, let's merge this.

We can remove the label-handling later when we're all ready for it.

Let's also consider the YML file names later, once the Windows one is also done.

And at the same time we can see about naming the "build_x" bit (the third part in "Tests / OS / build_x") in a nicer and more consistent way:

image

PS We'll need to backport all of these PRs to 3.11/3.12 -- but obviously not the free threaded part. And we can take care of that later too.

@hugovk hugovk merged commit c19561b into python:main Oct 30, 2023
@hugovk hugovk changed the title gh-111062: Reusable ubuntu build that supports free-threaded mode as the conditional CI gh-111062: Reusable Ubuntu build that supports free-threaded mode as the conditional CI Oct 30, 2023
@NCLI
Copy link
Contributor Author
NCLI commented Oct 30, 2023

Thanks for the help @corona10 :-)

FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
@NCLI @corona10 @FullteaR
pythongh-111062: Reusable ubuntu build that supports free-threaded mo…
b4d3b36 
…de as the conditional CI (python#111452)

Co-authored-by: Donghee Na <donghee.na92@gmail.com>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@NCLI @corona10 @aisk
pythongh-111062: Reusable ubuntu build that supports free-threaded mo…
29a07a6 
…de as the conditional CI (python#111452)

Co-authored-by: Donghee Na <donghee.na92@gmail.com>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@NCLI @corona10 @Glyphack
pythongh-111062: Reusable ubuntu build that supports free-threaded mo…
998bc16 
…de as the conditional CI (python#111452)

Co-authored-by: Donghee Na <donghee.na92@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk hugovk approved these changes

@corona10 corona10 corona10 approved these changes

@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner

Assignees

@hugovk hugovk

Labels
skip news topic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NCLI @hugovk @corona10
0