ci: configure bors

[messense]

Closes #2710
Closes #2714

[messense]

actions/runner#1985 matrix doesn't work in jobs.<job_id>.if, so annoying.

[messense]

bors try

[bors]

try

Build succeeded:

• conclusion

[adamreichold]

Is this really necessary? Doesn't needs already require these job to finish successfully? Also what the reason to have if: always() here?

Generally speaking, I am not a fan of the manual JavaScript-based approach to CI workflows and would prefer if just split our workflows into one pull requests and for used by bors even if it means having some redundancies between the two workflow definitions if this allows us to keep the declarative approach.

I am also not sure if just adjusting the matrix of the build job is sufficient differentiation between the two use cases (pre-review check and pre-merge check). More fundamentally, I feel that the build could use some re-engineering as it has a collected a lot of different use cases in conditional steps which would work better as individual jobs IMHO.

[messense]

Is this really necessary? Doesn't needs already require these job to finish successfully? Also what the reason to have if: always() here?

It gives a nicer overview: https://github.com/PyO3/pyo3/actions/runs/3508244920/jobs/5876898372#step:2:1 even when canceled due to previous failures.

Copied from https://github.com/cross-rs/cross/blob/ab99baf7f4c39403a0b033cef95d798bd0b9a940/.github/workflows/ci.yml#L361-L373

[messense]

I feel that the build could use some re-engineering as it has a collected a lot of different use cases in conditional steps which would work better as individual jobs IMHO.

Expanding more jobs may result in more cache misses thus longer build time, we are already over the 10GB cache limit.

[adamreichold]

Expanding more jobs may result in more cache misses thus longer build time, we are already over the 10GB cache limit.

We could also improve on this front, considering the approach outlined in #2710 (comment), the caches for the pull requests could become much more lightweight. (I think the issue of missing caches could also be solved by running the lightweight on-pull-request workflow as an on-push event on main? The heavyweight jobs would only run on trying/staging I guess?)

[messense]

run check (not test) for each OS / Python version, and then one run of the test suite on each OS w. latest Python

It's not really lightweight IMO, I'd drop the run check (not test) for each OS / Python version part, and coverage jobs are even heavier but they should be kept to ensure good test coverage.

I think the issue of missing caches could also be solved by running the lightweight on-pull-request workflow as an on-push event on main?

It will do, but I think with bors we shouldn't re-run workflow on main, bors was designed to avoid broken checks on main I think?. Maybe we should explore using sccache instead.

[adamreichold]

I'd drop the run check (not test) for each OS / Python version part

I agree. I don't think full platform coverage is something we need during review.

and coverage jobs are even heavier but they should be kept to ensure good test coverage.

As with the above, I think one coverage job should suffice. (And it could subsume the test job because the tests run to produce the coverage.)

It will do, but I think with bors we shouldn't re-run workflow on main,

Exactly, bors should only run on trying/staging which is main after merging the PR, hence one does not need to re-run bors on main because updating main to trying/staging is a fast-forward merge to something that was just tested.

So in summary:

• main and pull requests: lightweight pre-review workflow to seed cache.
• trying/staging: heavyweight bors pre-merge workflow. (Since bors pushes to our repo it should be able to use our caches without seeding.)

[messense]

bors try

[bors]

try

Build succeeded:

• conclusion

[messense]

bors try

[bors]

try

Build succeeded:

• conclusion

[davidhewitt]

Thanks! I'm curious to see what it looks like without the conclusion job, so I've pushed that to my own branch.

[messense]

I'm curious to see what it looks like without the conclusion job

I'm a bit worried about it due to this comment in bors forum: https://forum.bors.tech/t/check-matrix/673/2

Bors always marks things finished with the first status that matches the name. Whatever status is set in your bors.toml should be unique, otherwise things won't work, because bors doesn't know about your test matrix.

[davidhewitt]

Right, good find.

In which case, I think this is a great starting point, shall we merge this one and then we can write follow-up PRs for the improvements in #2761 (comment) as well as bringing pyo3-ffi-check into this workflow etc?

[messense]

In which case, I think this is a great starting point, shall we merge this one and then we can write follow-up PRs for the improvements in #2761 (comment) as well as bringing pyo3-ffi-check into this workflow etc?

I think so.

We need to tweak the required status. I think we can drop python3.11-* in required status first and use bors r+ command to generate the bors build status then add it to required status.

[messense]

bors r+

[davidhewitt]

I think I've managed to update the rules... Let's see what happens.

[bors]

Build succeeded:

• conclusion