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

Further ci updates - numeric tests, and run all tests on PRs #1369

Merged
merged 2 commits into from
Mar 9, 2024
Merged

Conversation

bluss
Copy link
Member

@bluss bluss commented Mar 9, 2024

Somehow, numeric test take 30m on powerpc, which is too slow (much slower than anything else.)
Now use s390x instead for big endian coverage, and release mode, which seems to use 15 minutes.

All the regular tests move back to PR checks to give contributors feedback on their PRs. It is more productive if contributors get access to the Rust 1.51 buildbot results immediately themselves.

@bluss bluss enabled auto-merge March 9, 2024 17:36
@bluss bluss added this pull request to the merge queue Mar 9, 2024
@bluss bluss removed this pull request from the merge queue due to a manual request Mar 9, 2024
@bluss bluss changed the title Skip numeric tests in cross tests Run numeric tests in release mode to speed them up Mar 9, 2024
@bluss bluss changed the title Run numeric tests in release mode to speed them up Run numeric cross tests in release mode to speed them up Mar 9, 2024
@adamreichold
Copy link
Collaborator

I think running them in release mode is a good call, especially with the CI caching the builds. But I also remember that this became significantly worse when I switched the big endian build from MIPS to PowerPC. I wonder if there is another big endian tier 2 target we could use which has faster emulation? s390x-unknown-linux-gnu because it is CISC and hence faster to emulate?

@bluss
Copy link
Member Author

bluss commented Mar 9, 2024

That sounds good - s390x- because even release mode powerpc is not so fast, also considering just not testing big endian anymore.

I'm still tinkering with the CI.

My current thought is that all the regular tests need to move back to PR checks, because it just encourages maintainer busywork if we have to be in the loop to give contributors feedback on their PRs. More productive if contributors get access to the Rust 1.51 buildbot results immediately themselves (because it's a tricky one, yeah it's too old, and nobody runs that on their machine.)

@adamreichold
Copy link
Collaborator

My current thought is that all the regular tests need to move back to PR checks, because it just encourages maintainer busywork if we have to be in the loop to give contributors feedback on their PRs. More productive if contributors get access to the Rust 1.51 buildbot results immediately themselves (because it's a tricky one, yeah it's too old, and nobody runs that on their machine.)

Agreed, the tests should be run on all PR, at least part of the matrix and your reasoning for 1.51 makes sense to me. I think the cross tests could still be limited to the merge queue. (I would also argue for keeping the merge queue as it makes the state of the main branch more reliable.)

That sounds good - s390x- because release mode is not so fast, also considering just not testing big endian anymore.

I would prefer keeping it and trying to make it faster for a bit more. Too much unsafe in the code base to pass any available testing. (I would even argue adding a cargo-careful run to the merge queue since we probably cannot use Miri with the FFI interactions.)

Somehow, these take 30m on powerpc, which is too slow (much slower than
anything else.)

Try to use s390x instead and run numeric tests in release mode.
We want feedback for contributors from the tests, directly on the PR.
@bluss bluss changed the title Run numeric cross tests in release mode to speed them up Further ci updates - numeric tests, and run all tests on PRs Mar 9, 2024
@bluss bluss added this pull request to the merge queue Mar 9, 2024
@bluss
Copy link
Member Author

bluss commented Mar 9, 2024

By default the merge queue runs many PRs concurrently, but for now I set it to sequential. Thanks for the help with this, now it's at least slightly modernized.

Merged via the queue into master with commit 58d60ee Mar 9, 2024
7 checks passed
@bluss bluss deleted the ci branch March 9, 2024 19:52
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.

2 participants