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-110793: add GitHub Actions job that runs tests after installing #110794

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Oct 13, 2023

Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@hugovk hugovk changed the title GH-110793: add Github action job that runs tests after installing GH-110793: add GitHub Actions job that runs tests after installing Oct 13, 2023
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author

FFY00 commented Oct 13, 2023

The job is now successful, this is ready for review. I'd still wait a little bit before merging, to see if there is any discussion on the issue.

Signed-off-by: Filipe Laíns <lains@riseup.net>
--with-openssl-rpath
- name: Build CPython out-of-tree
working-directory: ${{ env.CPYTHON_BUILDDIR }}
run: make -j4
Copy link
Member

Choose a reason for hiding this comment

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

Why do we assume that 4 is a good number (other build jobs also do this)?
Снимок экрана 2023-10-13 в 11 04 43

Should we switch to -j$(nproc) / $(sysctl -n hw.ncpu)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's the number of threads for Linux and Windows.

In this PR, I just copied the other jobs, so this comes from there. We can do the improvements you are suggesting (would also be nicer when running locally with act for eg), but it should probably be in a different PR.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed something for another PR, but worth investigating.

I've sometimes done a test on my fork with different numbers of jobs in a matrix.

For example, with macOS: make buildbottest TESTOPTS="-j${{ matrix.jobs }} -uall,-cpu"

Shows ~4 is about right:

image

@hugovk
Copy link
Member

hugovk commented Oct 15, 2023

@hugovk
Copy link
Member

hugovk commented Oct 15, 2023

Let's also consider the CI time.

This job is 26m, compared to 14m for the standard Ubuntu one. Plus we recently added 3 x CIFuzz which are 21-29m:

image

https://github.com/python/cpython/actions/runs/6505589398

@FFY00
Copy link
Member Author

FFY00 commented Oct 16, 2023

This job is 26m, compared to 14m for the standard Ubuntu one. Plus we recently added 3 x CIFuzz which are 21-29m:

I noticed make test adds a couple options, I think that's why it takes less time.

./python -u -W default -bb -E -m test --fast-ci --timeout= --dont-add-python-opts

Let's try adding them to the new job.

build_ubuntu and build_ubuntu_installed have a lot of duplicate lines: ~60/66 are identical.

build.yml is getting too long, it's over 700 lines!

If we go for this, let's do them as reusable workflows:

Agreed. Do you want to implement that in this PR or in a followup one?

@hugovk
Copy link
Member

hugovk commented Oct 16, 2023

It would be good in this PR.

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.

3 participants