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

github/workflows: attempt to enable CI for merge queue #15869

Draft
wants to merge 1 commit into
base: testing_queue
Choose a base branch
from

Conversation

jeeb
Copy link
Member

@jeeb jeeb commented Feb 14, 2025

No description provided.

@jeeb jeeb force-pushed the add_ci_to_merge_queue branch from 30dc1d1 to 73f9613 Compare February 14, 2025 23:13
@jeeb jeeb enabled auto-merge February 14, 2025 23:18
@jeeb jeeb added this pull request to the merge queue Feb 14, 2025
@jeeb jeeb removed this pull request from the merge queue due to a manual request Feb 14, 2025
Copy link

Download the artifacts for this pull request:

Windows
macOS

@kasper93
Copy link
Contributor

What's missing

  • require all the checks that we run. There is no reason to cherry-pick some
  • don't forget about https://github.com/mpv-player/mpv/blob/f94b44c8e5d4d15a5b102b13adba4ed25fb14128/.github/workflows/fuzz.yml
  • set merge queue to 2h, because if we starve our runners it may take longer than 1h, me think. openbsd build takes 15 minutes, fuzzing takes over 40 minutes. But there are a lot of linux runners 20, so we manage to eat this time, but I noticed win32 can sometimes also wait, because there are only 5 runners. Fuzzing can be made to 10 minutes, but the main bottleneck is the build time really, so it would be still be over 30 minutes. I guess we will see if this is a problem. It will build everything on queue again, so kinda more load...

@sfan5
Copy link
Member

sfan5 commented Feb 16, 2025

If I understand correctly this will invoke a new build for every PR to be merged.
However the fuzzing runs regularly take up to 1 hour. Adding so much delay is not acceptable.

Also that fact that we can't merge any PRs when msys inevitably breaks again in some way will come back to bite us.

@kasper93
Copy link
Contributor

kasper93 commented Feb 16, 2025

Adding so much delay is not acceptable.

Care to explain why? This feature is precisely designed, so you don't have to wait/look at the paint dry and just click "merge when ready" and forget. Also I never seen that we rush to merge any PR, where any delay would be critical for the workflow.

However the fuzzing runs regularly take up to 1 hour.

To be exact 47 minutes, 23 for build and 23 for fuzzing. Fuzzing could be reduced to 10 minutes like I said above. But this would still be 37 minutes, not much win. We have constant build time unfortunately.

when msys inevitably breaks again

in which case fixing CI should be priority probably. But anyway, we can make only some of the checks required. Though in practice this probably beats the purpose of this whole exercise, because if we allow accidental breaks, we might as well don't bother enabling it. I'm saying this in most positive way, there are few annoying things about this queue system that we might not want to fight with. And in practice, except little convenience it won't change the way we merge prs really...

EDIT: We could make it even other way around, do fuzzing jobs only on merge_queue trigger. Not sure if this is possible. Because from what I've seen few days ago is that, it waits for required checks on pull_request trigger, before starting merge. So likely both needs to pass. Which is another annoyance tbh.

@sfan5
Copy link
Member

sfan5 commented Feb 17, 2025

Care to explain why?

There's no process reason why, I was just thinking it's annoying to have such a large delay from "I want to merge this" to the commit actually appearing in the repo.
"so much delay is not acceptable" sounds very strong and I don't know why I wrote it like that

in which case fixing CI should be priority probably.

Priority sure but not a requirement. If I want to merge a small fix I should not be forced to figure out the msys issue or temporarily disable it.
I know I could just push it directly but I still think it's suboptimal.

@Dudemanguy
Copy link
Member

Dudemanguy commented Feb 17, 2025

Is it possible to ignore failures on certain steps in the job? Like if the build step fails, that's obviously a no-no and should be fixed. But if the container just derps out for some random reason, that's not a blocker.

@kasper93 kasper93 marked this pull request as draft February 24, 2025 15:28
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.

4 participants