-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add a disabled builder for aarch64 emulated tests #43226
Conversation
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
src/ci/shared.sh
Outdated
@@ -15,6 +15,12 @@ | |||
|
|||
# See http://unix.stackexchange.com/questions/82598 | |||
function retry { | |||
# Only do retries on CI, locally they shouldn't really be necessary | |||
if [ -z "$TRAVIS$APPVEYOR" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternate to avoid unbound variable issues: "${TRAVIS:-}${APPVEYOR:-}"
, though it's possible using $CI
would be cleaner.
.arg("-kernel").arg("/tmp/zImage") | ||
.arg("-initrd").arg(&rootfs_img) | ||
.arg("-dtb").arg("/tmp/vexpress-v2p-ca15-tc1.dtb") | ||
.arg("-append").arg("console=ttyAMA0 root=/dev/ram rdinit=/sbin/init init=/sbin/init") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tidy check (x86_64-unknown-linux-gnu)
[00:03:20] tidy error: /checkout/src/tools/remote-test-client/src/main.rs:157: line longer than 100 chars
[00:03:20] tidy error: /checkout/src/tools/remote-test-client/src/main.rs:169: line longer than 100 chars
[00:03:21] some tidy checks failed
src/ci/shared.sh
Outdated
"$@" | ||
return | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, the $CI
environment variable is not exported inside docker (https://github.com/rust-lang/rust/blob/4bd27fc/src/ci/docker/run.sh#L70-L87) so this will disable retries for any docker builds.
That aside, I'm reluctant to give users a different experience to what we work with in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to expand on my reluctance - one reason we do CI is so we know other people can build it correctly. I can imagine myself getting frustrated by an intermittent failure, just to realise that rust uses a magic variable to dodge it.
Is there a particular reason for changing it as part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops sorry I meant to respond to this earlier! I changed this here because it's a pretty bad experience when working on the docker files. Every failure turns into a "retry" so if you've got a normal failure in the docker image it just gives it to you 5 times which often makes it hard to find the error.
This seems contentious though so I'll just back it out.
☔ The latest upstream changes (presumably #43316) made this pull request unmergeable. Please resolve the merge conflicts. |
cc @rust-lang/infra, anybody up for giving this an official review? |
0f43c8c
to
0282dfe
Compare
I'll go through it this weekend (unless someone jumps in first). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is disabled I don't particularly mind r+ing this mostly as-is, but the CI
variable stuff needs tweaking in some way (either passing it down to docker, or removing the retry disabling) or I think we're going to start seeing spurious failures again.
cd /build/linux-4.4.42 && \ | ||
cp /build/.config . && \ | ||
make -j$(nproc) all && \ | ||
cp arch/arm64/boot/Image /tmp && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we use zImage
for armhf, it'd probably be nice if we did the same here just for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would yeah, but the normal build process didn't create that, so we didn't have it to use it :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should mention that I know nothing about kernel build processes, I just found that this particular configuration didn't create a zImage
RUN chmod +x rootfs/etc/init.d/rcS | ||
|
||
# Helper to quickly fill the entropy pool in the kernel. | ||
COPY armhf-gnu/addentropy.c /tmp/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and rcS
above are being pulled out of the armhf-gnu
folder - maybe a qemu-common folder?
src/ci/shared.sh
Outdated
"$@" | ||
return | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to expand on my reluctance - one reason we do CI is so we know other people can build it correctly. I can imagine myself getting frustrated by an intermittent failure, just to realise that rust uses a magic variable to dodge it.
Is there a particular reason for changing it as part of this PR?
1fe1437
to
eb6f996
Compare
Updated! |
RUN chmod +x rootfs/etc/init.d/rcS | ||
|
||
# Helper to quickly fill the entropy pool in the kernel. | ||
COPY armhf-gnu/addentropy.c /tmp/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one for scripts/
?
src/bootstrap/config.rs
Outdated
@@ -636,6 +636,12 @@ impl Config { | |||
let target = self.target_config.entry(target).or_insert(Target::default()); | |||
target.qemu_rootfs = Some(parse_configure_path(value)); | |||
} | |||
"CFG_QEMU_AARCH64_ROOTFS" if value.len() > 0 => { | |||
let target = "aarch64-unknown-linux-gnu".to_string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to use the interner in the new rustbuild, see the ARMHF line above. Looks like it changed underneath you!
r=me with the interning fix to get the PR build passing and the move of addentropy.c |
This commit adds a disabled builder which will run all tests for the standard library for aarch64 in a QEMU instance. Once we get enough capacity to run this on Travis this can be used to boost our platform coverage of AArch64
@bors: r=aidanhs |
📌 Commit 8ef3f69 has been approved by |
⌛ Testing commit 8ef3f69 with merge 1399c0d92e42e5510d784e3015e87c3c2eef7481... |
💔 Test failed - status-travis |
LLVM just stopped building... presumably sccache related? Not sure. cc @alexcrichton @bors retry
|
⌛ Testing commit 8ef3f69 with merge e0d7d4672073054e89eeebfc2287427305cb6301... |
💔 Test failed - status-travis |
⌛ Testing commit 8ef3f69 with merge e68b13ae6fab2d475a3d831257d85932867be3fe... |
💔 Test failed - status-travis |
Same error, should be legit. |
Third time in a row for that hang while building LLVM, but I can't figure out what's causing it given that virtually nothing has changed about armhf. My best guess is something to do with the docker build not being cached any more so some steps are re-run, but the only one that looks like it could change is the acquisition of @alexcrichton any thoughts? |
I have no idea how this PR could be related to that error. I'm running tests locally to try to replicate, I doubt I will be successful. |
@bors: retry Continue to be no failures locally, and I can't fathom how this is related to that... |
Add a disabled builder for aarch64 emulated tests This commit adds a disabled builder which will run all tests for the standard library for aarch64 in a QEMU instance. Once we get enough capacity to run this on Travis this can be used to boost our platform coverage of AArch64
☀️ Test successful - status-appveyor, status-travis |
This commit adds a disabled builder which will run all tests for the standard
library for aarch64 in a QEMU instance. Once we get enough capacity to run this
on Travis this can be used to boost our platform coverage of AArch64