From 6d815bbd3ef1d2ff98a40770f9b673f611e1884e Mon Sep 17 00:00:00 2001 From: Patrick Mooney Date: Thu, 21 Sep 2023 17:36:54 -0500 Subject: [PATCH] Revert abort-on-panic in 'dev' cargo profile Since proper unwinding is required for handling `should_panic` tests cases, the existing default of abort-on-panic was rather inconvenient. This returns the 'dev' profile to using unwind-on-panic, and cleans up the 'phd' profile, which can use 'dev' as well for its unwinding. --- .github/buildomat/jobs/image.sh | 3 +++ .github/buildomat/jobs/phd-build.sh | 16 ++++++++-------- Cargo.toml | 15 ++++++++------- phd-tests/README.md | 10 +++++----- phd-tests/quickstart.sh | 2 +- 5 files changed, 25 insertions(+), 21 deletions(-) diff --git a/.github/buildomat/jobs/image.sh b/.github/buildomat/jobs/image.sh index bbbe80607..d66ee29f0 100755 --- a/.github/buildomat/jobs/image.sh +++ b/.github/buildomat/jobs/image.sh @@ -30,6 +30,9 @@ banner build # Enable the "omicron-build" feature to indicate this is an artifact destined # for production use on an appropriately configured Oxide machine +# +# The 'release' profile is configured for abort-on-panic, so we get an +# immediate coredump rather than unwinding in the case of an error. ptime -m cargo build --release --verbose -p propolis-server --features omicron-build banner image diff --git a/.github/buildomat/jobs/phd-build.sh b/.github/buildomat/jobs/phd-build.sh index e27cc0fd4..8f5a8578c 100644 --- a/.github/buildomat/jobs/phd-build.sh +++ b/.github/buildomat/jobs/phd-build.sh @@ -17,22 +17,22 @@ outdir="/out" cargo --version rustc --version -# Build the Propolis server binary in debug mode to enable assertions -# that should fire during tests. +# Build the Propolis server binary with 'dev' profile to enable assertions that +# should fire during tests. banner build-propolis ptime -m cargo build --verbose -p propolis-server -# Build the PHD runner with the phd profile to enable unwind on panic, -# which the framework requires to catch certain test failures. +# The PHD runner requires unwind-on-panic to catch certain test failures, so +# build it with the 'dev' profile which is so configured. banner build-phd -ptime -m cargo build --verbose -p phd-runner --profile=phd +ptime -m cargo build --verbose -p phd-runner banner contents tar -czvf target/debug/propolis-server-debug.tar.gz \ -C target/debug propolis-server -tar -czvf target/phd/phd-runner.tar.gz \ - -C target/phd phd-runner \ +tar -czvf target/debug/phd-runner.tar.gz \ + -C target/debug phd-runner \ -C phd-tests artifacts.toml banner copy @@ -40,7 +40,7 @@ pfexec mkdir -p $outdir pfexec chown "$UID" $outdir mv target/debug/propolis-server-debug.tar.gz \ $outdir/propolis-server-debug.tar.gz -mv target/phd/phd-runner.tar.gz $outdir/phd-runner.tar.gz +mv target/debug/phd-runner.tar.gz $outdir/phd-runner.tar.gz cd $outdir digest -a sha256 propolis-server-debug.tar.gz > \ propolis-server-debug.sha256.txt diff --git a/Cargo.toml b/Cargo.toml index 131313c99..c01029f95 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,17 +27,18 @@ exclude = [ "phd-tests/buildomat", ] -[profile.dev] +# If one wants the 'dev' profile, but with "panic = abort" semantics, they +# should opt in with this profile. Unwinding is required by PHD and +# should_abort cargo tests, and so remains the default for the 'dev' profile. +[profile.dev-abort] +inherits = "dev" panic = "abort" + +# Building for 'release' implies running on a real illumos system, where we +# certainly want (rust) panics to cause an immediate abort and coredump. [profile.release] panic = "abort" -# The PHD test runner needs to use unwinding to catch panics that occur during -# tests (e.g. due to a failed `assert!` in a test case). -[profile.phd] -inherits = "dev" -panic = "unwind" - [workspace.dependencies] # Internal crates bhyve_api = { path = "crates/bhyve-api" } diff --git a/phd-tests/README.md b/phd-tests/README.md index 9013654f9..7fb1477e6 100644 --- a/phd-tests/README.md +++ b/phd-tests/README.md @@ -34,15 +34,15 @@ against them. To build: -`cargo build -p phd-runner --profile=phd` +`cargo build -p phd-runner` -Note that `--profile=phd` is required to allow the runner to catch assertions -from test cases (the default Propolis profile aborts on panic instead of -unwinding). +PHD requires the unwinding of stacks in order to properly catch assertions in +test cases, so building with a profile which sets `panic = "abort"` is not +supported. This precludes the use of the `release` or `dev-abort` profiles. To run: -`pfexec cargo run -p phd-runner --profile=phd -- [OPTIONS]` +`pfexec cargo run -p phd-runner -- [OPTIONS]` Running under pfexec is required to allow PHD to ensure the host system is correctly configured to run live migration tests. diff --git a/phd-tests/quickstart.sh b/phd-tests/quickstart.sh index 9f19814b9..75f00fe17 100755 --- a/phd-tests/quickstart.sh +++ b/phd-tests/quickstart.sh @@ -10,7 +10,7 @@ if [ ! -d "$PHD_QUICKSTART_DIR" ]; then mkdir $PHD_QUICKSTART_DIR fi -pfexec cargo run --profile=phd -p phd-runner -- \ +pfexec cargo run -p phd-runner -- \ run \ --artifact-toml-path ./artifacts.toml \ --tmp-directory $PHD_QUICKSTART_DIR \