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

CI tweaks #221

Merged
merged 9 commits into from
Nov 21, 2022
Merged

CI tweaks #221

merged 9 commits into from
Nov 21, 2022

Conversation

dnicolodi
Copy link
Member

@dnicolodi dnicolodi commented Nov 20, 2022

The main goal here is to cut down the total number of jobs we run from 41 to to something more sensible.

I got them down to:

  • CPython 3.7 and 3.11, and on Windows, macOS, and Ubuntu: 6 jobs
  • CPython 3.8, 3.9, 3.10, and PyPy 3.9 on Ubuntu: 4 jobs
  • CPython 3.9 in Cygwin: 1 job
  • Pyston 3.8 on Ubuntu: 1 job
  • CPython 3.7 and 3.11 on Homebrew: 2 jobs
  • a mypy job,
  • the precommit tests,
  • the Sage build job and the 8 Sage test jobs.

That's 25 jobs total.

I've experimented with running the Sage workflow only of the tests workflow succeeds but that has the annoying consequence of removing the Sage workflow test results from the PR view. The only way I know to avoid this is to have the Sage jobs defined in the same workflow as the other tests, but it does not seem very tidy.

FORCE_COLOR is not useful now that nox is not involved anymore.
Test all supported Python versions only on Linux.
Until we don't make use of them, these just contribute to the total
number of jobs run, slowing down the jobs for all the organization
when the maximum number of concurrent jobs is hit.
@FFY00
Copy link
Member

FFY00 commented Nov 20, 2022

Would it be possible to run everything on the main branch, but a fewer jobs on PRs? Sometimes there are version specific issues, that we still want to catch, but as they are not common enough it might be hard to justify doing that in PRs.

@dnicolodi
Copy link
Member Author

Sure, but you would most likely end up enabling the skipped tests in the PR that tries to fix the issues founds on main. Not a big deal, if infrequent enough. We could have the Sage tests run on push to main only. These are the ones that take the most time and are unlikely to catch anything not caught by the other tests.

@FFY00
Copy link
Member

FFY00 commented Nov 20, 2022

I think the Sage tests are useful in PRs, as they test real work scenarios, but I don't think we need these many in PRs. We could have only archlinux and conda-forge, for eg.

@dnicolodi
Copy link
Member Author

Can do that. Not running the gentoo test on PRs is already a big saving.

@dnicolodi dnicolodi force-pushed the ci-tweaks branch 4 times, most recently from ff7d437 to 4c172ab Compare November 20, 2022 17:21
@dnicolodi
Copy link
Member Author

The workflow definition look a bit convoluted, but it achieves what we want. I'm surprised that the Sage jobs take so long, but that's a fight for another time. @FFY00 Unless there is something you don't like, I'll like to merge this soon as it is a big improvement upon the current status.

@FFY00
Copy link
Member

FFY00 commented Nov 21, 2022

@dnicolodi yes, sounds good. I am gonna tag a release, feel free to merge afterwards.

@dnicolodi
Copy link
Member Author

I would appreciate having the documentation for the new options before the release. But I can live without that.

@FFY00
Copy link
Member

FFY00 commented Nov 21, 2022

I did not do that because we really needed a release, and we are planning a documentation rewrite anyway (#197) 😅

@dnicolodi dnicolodi merged commit 2564ce8 into mesonbuild:main Nov 21, 2022
@@ -253,8 +253,8 @@ jobs:
- name: Run mypy
run: mypy -p mesonpy

tests-pass:
Copy link

Choose a reason for hiding this comment

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

@dnicolodi moreover — this implementation was broken anyway.

https://github.com/marketplace/actions/alls-green#why

Copy link
Contributor

Choose a reason for hiding this comment

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

It was not faulty. It just gets marked as skip instead of fail, but it's required to merge, so that's the same result. It would be better if it always ran, and reported the result (via alls-green) probably, but it still lets you do things like "merge when CI passes".

Why was this commented out? It doesn't slow the job by reducing parallism since it always runs at the end. So there's an extra at most 7 seconds or something tacked on the end. And it lets you makes the "passing" requirement in GitHub much simpler, and changes to it don't cause previous jobs to turn orange.

Copy link

@webknjaz webknjaz Dec 2, 2022

Choose a reason for hiding this comment

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

but it still lets you do things like "merge when CI passes".

Not exactly. It also lets you do things “merge when CI fails” because skipped==pass in branch protection if you start relying on this implementation.

Copy link

Choose a reason for hiding this comment

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

It was not faulty.

Read the whole page at https://github.com/marketplace/actions/alls-green#why for explanation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahah, so that action is removing the "merge when CI fails" feature. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Why was this commented out? It doesn't slow the job by reducing parallism since it always runs at the end. So there's an extra at most 7 seconds or something tacked on the end. And it lets you makes the "passing" requirement in GitHub much simpler, and changes to it don't cause previous jobs to turn orange.

I commented it out because it is not used. Because all the projects within the mesonbuild organization share the GitHub Actions runners quota, it happened several times that the first batch of jobs from the meson-python is queued then some long-running jobs from the meson or wrapdb projects are queued and the summary job is left waiting for a long time for a runner just to execute an echo.

And it lets you makes the "passing" requirement in GitHub much simpler

In general I don't think that having something enforcing the policy that a PR is to be merged only when the test suite passes is very useful. I think we can trust our pairs to behave. The test suite was failing on several configurations and these tests were simply excluded from the required set. The fact that merging was allowed made no one paid attentions to what was actually a bug. Just merge when all the tests are green is a much simpler and much more effective policy.

changes to it don't cause previous jobs to turn orange

I don't know what you mean by this. I don't think the status of open PRs changes when the CI jobs definitions on the main branch change. If it would, that would be a welcome feature: PRs should be tested against the current main branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants