Add QEMU test for x86_64-unknown-uefi #101703

nicholasbishop · 2022-09-11T22:18:32Z

The UEFI targets don't have std support yet, so the normal tests don't work. However, we can compile a simple no-std program and run it under QEMU to at least check that the target compiles, links, and runs.

Tested locally with: src/ci/docker/run.sh x86_64-uefi

rust-highfive · 2022-09-11T22:18:35Z

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

nicholasbishop · 2022-09-11T22:21:38Z

I don't have any prior knowledge of the Rust CI, and I didn't see any existing CI jobs that looked like a good analog for what I was trying to do here. So, I'm not sure if this is the right way to go about it -- any feedback appreciated :)

CC @dvdhrm

jyn514 · 2022-09-11T23:20:15Z

The tests themselves look vaguely right, although I think using QEMU is rather unusual.

What tier is UEFI? I was under the impression it's tier 3, which we don't test.

nicholasbishop · 2022-09-11T23:23:33Z

You are correct that UEFI is currently tier 3, however there is an open proposal to move to tier 2: rust-lang/compiler-team#555

jyn514 · 2022-09-11T23:27:29Z

Ok. The way the tier system works is that tier 3 has support but no CI, tier 2 is built in CI but not tested, and only tier 1 is tested. So I can't accept this PR unless UEFI is tier 1 (which seems unlikely). Once your MCP is accepted, you can change it to build only and we can merge that.

jyn514 · 2022-09-11T23:28:12Z

cc @joshtriplett to confirm I have the tier policy correct - only tier 1 has tests, right?

dvdhrm · 2022-09-12T07:37:49Z

(from https://doc.rust-lang.org/nightly/rustc/target-tier-policy.html)

Tier-2

The target must build reliably in CI, for all components that Rust's CI considers mandatory.

The approving teams may additionally require that a subset of tests pass in CI, such as enough to build a functional "hello world" program, ./x.py test --no-run, or equivalent "smoke tests". In particular, this requirement may apply if the target builds host tools, or if the tests in question provide substantial value via early detection of critical problems.

@jyn514, my interpretation is that basic target tests are allowed for Tier-2 targets, explicitly mentioning a hello-world program as suggested here. It is, however, unclear to me whether this requires approval by a team. The wording only defines the inverse, in that if it is asked for, it must be provided. Maybe Josh can shed some light on this.

jyn514 · 2022-09-12T13:41:54Z

Ok, in that case I'll marked this as blocked on rust-lang/compiler-team#555.

dvdhrm

Thanks a lot for submitting this! Looks all good to me, a few minor questions inline.

src/ci/docker/host-x86_64/x86_64-uefi/uefi_qemu_test/src/main.rs

src/ci/docker/host-x86_64/x86_64-uefi/uefi_qemu_test/run.py

the8472 · 2022-09-13T12:15:32Z

The tests themselves look vaguely right, although I think using QEMU is rather unusual.

We already have some ARM tests running under emulation on x86 CI.

the8472 · 2022-09-13T12:24:41Z

.github/workflows/ci.yml

+          - name: x86_64-uefi
+            os: ubuntu-20.04-xl
+            env: {}


This might fit into the test-various job? That currently 37minutes so it's one of the shorter-running jobs.

@nicholasbishop can you please make this change?

Done.

One thing to note is that the test-various Dockerfile is using Ubuntu 20.04, which provides a quite old QEMU (4.2.1). That version of QEMU likes to crash on exit with OMVF, we ran into that problem in the CI for uefi-rs in the past. I changed the test to not check the exit status of the VM, which I think is fine since we're checking the stdout for a specific string anyway.

jyn514 · 2022-09-20T14:19:30Z

src/ci/docker/host-x86_64/x86_64-uefi/uefi_qemu_test/run.py

+def build_and_run(tmp_dir):
+    host_artifacts = Path('/checkout/obj/build/x86_64-unknown-linux-gnu')
+    stage0 = host_artifacts / 'stage0/bin'
+    stage2 = host_artifacts / 'stage2/bin'


Hard coding the target and directory structure here makes me a little nervous ... I guess if it breaks it will break quickly in CI. It would still make me more comfortable to make this managed by bootstrap, but I won't block on that.

jyn514 · 2022-09-20T14:22:07Z

r=me with #101703 (comment) done and the MCP accepted :)

The UEFI targets don't have std support yet, so the normal tests don't work. However, we can compile a simple no-std program and run it under QEMU to at least check that the target compiles, links, and runs. Tested locally with: src/ci/docker/run.sh test-various

nicholasbishop · 2022-11-03T15:11:17Z

@jyn514 the MCP has been accepted :)

jyn514 · 2022-11-03T15:14:53Z

@bors r+ rollup=iffy

bors · 2022-11-03T15:14:55Z

📌 Commit 1e264ab has been approved by jyn514

It is now in the queue for this repository.

nicholasbishop · 2022-11-04T15:48:37Z

Is bors not working correctly here? https://bors.rust-lang.org/queue/rust doesn't show this PR as approved.

jyn514 · 2022-11-04T16:14:08Z

ugh

@bors r- r+

bors · 2022-11-04T16:14:12Z

📌 Commit 1e264ab has been approved by jyn514

It is now in the queue for this repository.

bors · 2022-11-04T17:00:28Z

⌛ Testing commit 1e264ab with merge c2a5c3a...

bors · 2022-11-04T19:43:30Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing c2a5c3a to master...

rust-timer · 2022-11-04T22:20:21Z

Finished benchmarking commit (c2a5c3a): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.8%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-1.0%, 0.8%]	4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	2
Improvements ✅ (secondary)	-4.4%	[-4.4%, -4.4%]	1
All ❌✅ (primary)	-0.2%	[-0.6%, 0.5%]	3

rust-timer · 2022-11-04T23:01:43Z

Finished benchmarking commit (c2a5c3a): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.8%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-1.0%, 0.8%]	4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	2
Improvements ✅ (secondary)	-4.4%	[-4.4%, -4.4%]	1
All ❌✅ (primary)	-0.2%	[-0.6%, 0.5%]	3

rust-highfive assigned jyn514 Sep 11, 2022

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Sep 11, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2022

nicholasbishop force-pushed the bishop-add-uefi-ci-2 branch from 782cafa to b9289ce Compare September 11, 2022 22:27

jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. O-UEFI UEFI and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2022

dvdhrm reviewed Sep 12, 2022

View reviewed changes

the8472 reviewed Sep 13, 2022

View reviewed changes

nicholasbishop force-pushed the bishop-add-uefi-ci-2 branch 2 times, most recently from a34dbf0 to be48f46 Compare September 15, 2022 16:39

dvdhrm approved these changes Sep 20, 2022

View reviewed changes

jyn514 reviewed Sep 20, 2022

View reviewed changes

nicholasbishop force-pushed the bishop-add-uefi-ci-2 branch from be48f46 to 1e264ab Compare September 20, 2022 23:11

nicholasbishop mentioned this pull request Sep 25, 2022

Switch the toolchain back to latest nightly rust-osdev/uefi-rs#516

Merged

2 tasks

bors removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 3, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 3, 2022

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 4, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 4, 2022

bors merged commit c2a5c3a into rust-lang:master Nov 4, 2022

rustbot added this to the 1.67.0 milestone Nov 4, 2022

nicholasbishop deleted the bishop-add-uefi-ci-2 branch November 4, 2022 19:45

the8472 mentioned this pull request Feb 3, 2023

Consider simulation testing for math on 16-bit targets #107612

Open

Add QEMU test for x86_64-unknown-uefi #101703

Add QEMU test for x86_64-unknown-uefi #101703

Conversation

nicholasbishop commented Sep 11, 2022

Uh oh!

rust-highfive commented Sep 11, 2022

Uh oh!

nicholasbishop commented Sep 11, 2022

Uh oh!

jyn514 commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicholasbishop commented Sep 11, 2022

Uh oh!

jyn514 commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyn514 commented Sep 11, 2022

Uh oh!

dvdhrm commented Sep 12, 2022

Uh oh!

jyn514 commented Sep 12, 2022

Uh oh!

dvdhrm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

the8472 commented Sep 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyn514 commented Sep 20, 2022

Uh oh!

nicholasbishop commented Nov 3, 2022

Uh oh!

jyn514 commented Nov 3, 2022

Uh oh!

bors commented Nov 3, 2022

Uh oh!

nicholasbishop commented Nov 4, 2022

Uh oh!

jyn514 commented Nov 4, 2022

Uh oh!

bors commented Nov 4, 2022

Uh oh!

bors commented Nov 4, 2022

Uh oh!

bors commented Nov 4, 2022

Uh oh!

rust-timer commented Nov 4, 2022

Overall result: ❌ regressions - no action needed

Uh oh!

rust-timer commented Nov 4, 2022

Overall result: ❌ regressions - no action needed

Uh oh!

Uh oh!

jyn514 commented Sep 11, 2022 •

edited

Loading

jyn514 commented Sep 11, 2022 •

edited

Loading