Use the build job name instead of success in ci-checks for rustc_codegen_gcc

[antoyo]

The job name should be build and not success.

[antoyo]

I wonder if this is the correct solution, since we have multiple workflow files.
Any idea?

[Kobzol]

Do you want all the workflows to succeed for a PR to be merged? In general, for a branch protection, you need to specify job names that have to succeed. Usually, if there are multiple jobs in a workflow, we create a single "success" job that merges their results together.

If there are multiple workflows, there would have to be multiple success jobs per workflow (or maybe it's enough to name them all in the same way? not sure how that works).

I think that build on its own will not work here, because the build job actually uses a CI matrix, so it will actually generate a bunch of jobs, which will all be named differently. In that case it's best to create a success job to make the branch protection easier.

Here is a PR that adds a success job to the (main?) ci.yml workflow.

[tgross35]

If you expect them to all be successful, I think you can add another .github/workflow job that depends on all the other.yaml files https://stackoverflow.com/a/64733705/5380651.

[Kobzol]

I don't think that we use that anywhere in rust-lang for marking CI success, but we could test drive it here 😆 https://github.com/rust-lang/rust-bindgen/blob/59a43e10b758bd86275aefceae29e874157087d8/.github/workflows/publish.yml#L4 is an example where it is used to publish crates to crates.io.

[tgross35]

How nice would it be if GH just had an easy way to do "required: all jobs" 🙃

[antoyo]

If you expect them to all be successful, I think you can add another .github/workflow job that depends on all the other.yaml files https://stackoverflow.com/a/64733705/5380651.

From what I can see in the comments, this will only be ran on the default branch, so I believe this would not work for what we want to do.
Am I missing something?

[tgross35]

Oh, I don't know then. Maybe it works if branches: [main] is omitted? I haven't actually used this either.

If not, would it be feasible to combine the workflows that need to run for a PR to a single file?

[tgross35]

I guess there isn't much harm in just trying the PR here either but being ready to revert if GH doesn't like it. When multiple jobs have the same name the gh CLI tool also needs the job file name to disambiguate, but branch protection won't care that there are multiple jobs named build?

[antoyo]

Oh, I don't know then. Maybe it works if branches: [main] is omitted? I haven't actually used this either.

I tried in a test repo and that doesn't work since the job is not in the PR itself, so the job doesn't pass required check.

---

I was about to gave up when I found this action. You can see in this PR in my test repo the "required-checks" job that checks that all jobs across all workflows that contain "build" (as configured here) passed.

It works by polling at 30 second-intervals (this timing is configurable), though. Do you think that would be a problem?

Would that solution be good enough for you?

[Kobzol]

I think that there's a simpler way, without the polling hack. I just tried this in my test repo. If you create a job with the same name (e.g. conclusion or success) in each workflow that you want to have as required, GH will only allow merging the PR if all these jobs (in all the required workflows) will be successful.

[antoyo]

So, with this PR, everything should be working without any more changes (even in the team repo)?

[Kobzol]

I think so. In fact, since the rest of the workflows seem to only have one job, you could also just rename that job to success, or change the branch protection to ["success", "build"].

[antoyo]

I just tried that, but it seems success only works if there are no matrices.

[Kobzol]

Ah, yes, as I wrote earlier, if there is a matrix, a conclusion job is needed, even if there is only a single job (using the matrix) in the workflow. Sorry.

[antoyo]

Is it possible that we should give each work "success" job a different name in the different workflows and add them all here?
Because it seems I can merge this PR even though it's missing a few "success" jobs to finish.

[Kobzol]

It seems to work correctly, all the success jobs are marked as "Required". Btw, the repo runs all CI twice unnecessarily. If you're not using a merge queue, there shouldn't be a need for the on: push trigger. rust-lang/rustc_codegen_gcc#619 should fix this.

[antoyo]

It seems to work correctly, all the success jobs are marked as "Required".

No, while it seems that way in the end, the success jobs do not appear immediately. As soon as there's all "success" jobs that have appeared so far and they passed, it is possible to merge.

[Kobzol]

the success jobs do not appear immediately

Yeah, this is caused by the usage of the conclusion job with needs. A bit annoying, but I don't think it can be avoided.

As soon as there's all "success" jobs that have appeared so far and they passed, it is possible to merge.

Ah, I see now. Yeah, that's a problem. If the jobs with the same name weren't using needs, then maybe it would work, but in this case it's problematic.

In that case using differently named conclusion jobs is probably the way to go, sigh...

[antoyo]

In that case using differently named conclusion jobs is probably the way to go, sigh...

Do you prefer that or the other (polling) solution I mentioned above?

[Kobzol]

I think it's much simpler (to understand/debug) to just use differently named jobs.

[antoyo]

Ok, I merged this PR with the different job names and I updated this PR to include those names.

If this becomes a problem later, we'll think about a better solution (or hope that GitHub provides a better way).

[antoyo]

Just in case this wasn't clear: this PR is ready for review and to be merged.

[Kobzol]

@Mark-Simulacrum Could you take a look, please? Thanks!