Skip to content

Commit

Permalink
Revert abort-on-panic in 'dev' cargo profile
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pfmooney committed Sep 25, 2023
1 parent de6369a commit 6d815bb
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 21 deletions.
3 changes: 3 additions & 0 deletions .github/buildomat/jobs/image.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions .github/buildomat/jobs/phd-build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,30 @@ 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
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
Expand Down
15 changes: 8 additions & 7 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
10 changes: 5 additions & 5 deletions phd-tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion phd-tests/quickstart.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down

0 comments on commit 6d815bb

Please sign in to comment.