chore: migrate from Cirrus CI to GHA#5058
Conversation
|
NetBSD CDN is down but it's unrelated to this change, ready for review. |
There was a problem hiding this comment.
Looks great to me, but maybe hold off for a day in case @asomers gets the chance to take a look.
Also maybe update the commit/PR description to say it's migrating to GHA? From the quick read I assumed you were just deleting the job :)
| mem: 4096 | ||
| copyback: false | ||
| prepare: | | ||
| set -eux |
There was a problem hiding this comment.
Why the "-u" ?
There was a problem hiding this comment.
I think it's good practice to always set this in scripts. Expected nullable variables are rare enough to have :- on a case-by-case basis.
There was a problem hiding this comment.
I guess I copied it from somewhere while referencing a prior-art PR 🙇
Addressed in 8c3f4fe
There was a problem hiding this comment.
Oh I was saying the other way around, -u should always be included. Not that it's super useful here since the only thing to catch is unset $HOME (which is actually a problem we've historically hit with some runners).
JohnTitor
force-pushed
the
retire-cirrus
branch
from
11836d2 to
8c3f4fe
Compare
JohnTitor
changed the title
Renamed, it was too cool :P |
| rustup target add i686-unknown-freebsd | ||
| fi | ||
| run: | | ||
| set -x |
There was a problem hiding this comment.
I think you still want -e here, or else the job will be considered passing even if the first run.sh invocation fails.
| set -x | |
| set -xe |
There was a problem hiding this comment.
Should I apply it to NetBSD scripts? I guess it benefits on it as well.
There was a problem hiding this comment.
Should I apply it to NetBSD scripts? I guess it benefits on it as well.
Yes, please do. Or, just remove the set -x, because the default is equivalent to -e.
There was a problem hiding this comment.
Yes, please do. Or, just remove the
set -x, because the default is equivalent to-e.
Why would this make a difference? set only changes the specified options and doesn't override others, unless GH is doing some inspection here.
Not that I think it's a bad thing to be explicit.
JohnTitor
force-pushed
the
retire-cirrus
branch
from
8c3f4fe to
326bbfd
Compare
| if [ "${{ matrix.target }}" = "i686-unknown-freebsd" ]; then | ||
| rustup target add i686-unknown-freebsd | ||
| fi |
There was a problem hiding this comment.
| if [ "${{ matrix.target }}" = "i686-unknown-freebsd" ]; then | |
| rustup target add i686-unknown-freebsd | |
| fi | |
| rustup target add ${{ matrix.target }} |
There was a problem hiding this comment.
On others the default host toolchain works. But also no harm since rustup does the same check, 🤷♂️
There was a problem hiding this comment.
It's less code, and would make things future-proof in case targets like aarch64-unknown-freebsd are added to the job matrix later.
There was a problem hiding this comment.
The funny thing about i686-unknown-freebsd is that we don't run that check in an i686 VM. We run it in an x86_64 VM, using 32-bit compat. So the rustup target add really is needed.
There was a problem hiding this comment.
Right, so rustup target add wouldn't be needed for aarch64-unknown-freebsd; i686-unknown-freebsd really is a special case.
There was a problem hiding this comment.
LGTM with or without @xtqqczze's suggestion, thank you for taking care of this!
tgross35
added
the
stable-nominated
JohnTitor
force-pushed
the
retire-cirrus
branch
from
326bbfd to
4eaaff5
Compare
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - name: Test on FreeBSD | ||
| uses: vmactions/solaris-vm@v1.3.2 |
There was a problem hiding this comment.
Oops, looks like a copy/paste error.
| uses: vmactions/solaris-vm@v1.3.2 | |
| uses: vmactions/freebsd-vm@v1 |
There was a problem hiding this comment.
Ah, good catch, fixed!
JohnTitor
force-pushed
the
retire-cirrus
branch
from
4eaaff5 to
fdc5e8d
Compare
5 participants
Description
Fix #5052
This migrates FreeBSD CI from Cirrus CI to GitHub Actions to address Cirrus CI shutdown.
The new GHA matrix should have the same workflow as Cirrus as possible.
Sources
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI