From d013ac9a25672f1eb89c8a9fb07f08563f7b08d9 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 May 2024 14:40:48 -0400 Subject: [PATCH 1/4] test(profile): show panic=abort doesnt work for custom harness --- tests/testsuite/profiles.rs | 61 +++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index b3a9915940c..fe39e9e142b 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -396,6 +396,67 @@ fn profile_panic_test_bench() { .run(); } +#[cargo_test] +fn profile_panic_test_with_custom_harness() { + // Custom harness can have `-C panic="…"` passed in. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + [[test]] + name = "custom" + harness = false + [[test]] + name = "libtest" + [[bench]] + name = "custom" + harness = false + [[bench]] + name = "libtest" + [profile.test] + panic = "abort" + [profile.bench] + panic = "abort" + "#, + ) + .file("src/lib.rs", "") + .file("tests/custom.rs", r#"fn main() { panic!("abort!"); }"#) + .file("tests/libtest.rs", "") + .file("benches/custom.rs", r#"fn main() { panic!("abort!"); }"#) + .file("benches/libtest.rs", "") + .build(); + + // panic abort on custom harness + p.cargo("test --test custom --verbose") + .with_stderr_does_not_contain("[..]panic=abort[..]") + .with_stderr_contains("[..]thread '[..]' panicked at [..]") + .with_stderr_does_not_contain( + "[..]process didn't exit successfully: `[..]/target/debug/deps/custom-[..]", + ) + .with_status(101) + .run(); + p.cargo("bench --bench custom --verbose") + .with_stderr_does_not_contain("[..]panic=abort[..]") + .with_stderr_contains("[..]thread '[..]' panicked at [..]") + .with_stderr_does_not_contain( + "[..]process didn't exit successfully: `[..]/target/release/deps/custom-[..]", + ) + .with_status(101) + .run(); + + // panic behaviour of libtest cannot be set as `abort` as of now. + p.cargo("test --test libtest --verbose") + .with_stderr_does_not_contain("panic=abort") + .with_stdout_contains("running 0 tests") + .run(); + p.cargo("bench --bench libtest --verbose") + .with_stderr_does_not_contain("panic=abort") + .with_stdout_contains("running 0 tests") + .run(); +} + #[cargo_test] fn profile_doc_deprecated() { let p = project() From df5948956d60f88c5702b0fb46e329f84bf89b5d Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 May 2024 14:50:29 -0400 Subject: [PATCH 2/4] feat(profile): panic behavior can be specified for custom harness It's libtest that requires tests to be always unwound. For custom harnesses they don't need such a requirement. Cargo relaxes it a bit in profile settings. --- src/cargo/core/compiler/unit_dependencies.rs | 4 +- src/cargo/core/profiles.rs | 39 +++++++++++++------ src/cargo/ops/cargo_compile/unit_generator.rs | 2 +- tests/testsuite/profiles.rs | 19 ++++++--- 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 1690259747a..840a840a1f4 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -213,9 +213,9 @@ fn deps_of_roots(roots: &[Unit], state: &mut State<'_, '_>) -> CargoResult<()> { if unit.target.proc_macro() { // Special-case for proc-macros, which are forced to for-host // since they need to link with the proc_macro crate. - UnitFor::new_host_test(state.gctx, root_compile_kind) + UnitFor::new_host_test(state.gctx, &unit.target, root_compile_kind) } else { - UnitFor::new_test(state.gctx, root_compile_kind) + UnitFor::new_test(state.gctx, &unit.target, root_compile_kind) } } else if unit.target.is_custom_build() { // This normally doesn't happen, except `clean` aggressively diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index ce2c9e414f6..47821b6af9b 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -1109,19 +1109,28 @@ impl UnitFor { /// whether `panic=abort` is supported for tests. Historical versions of /// rustc did not support this, but newer versions do with an unstable /// compiler flag. - pub fn new_test(gctx: &GlobalContext, root_compile_kind: CompileKind) -> UnitFor { + /// + /// Moreover, `target` is taken here for determining whether the test is + /// driven by libtest harness. Cargo relaxes the panic behaviour if it is + /// a custom harness, which is not required to be always unwound. + pub fn new_test( + gctx: &GlobalContext, + target: &Target, + root_compile_kind: CompileKind, + ) -> UnitFor { + // We're testing out an unstable feature (`-Zpanic-abort-tests`) + // which inherits the panic setting from the dev/release profile + // (basically avoid recompiles) but historical defaults required + // that we always unwound. + let panic_setting = if gctx.cli_unstable().panic_abort_tests || !target.harness() { + PanicSetting::ReadProfile + } else { + PanicSetting::AlwaysUnwind + }; UnitFor { host: false, host_features: false, - // We're testing out an unstable feature (`-Zpanic-abort-tests`) - // which inherits the panic setting from the dev/release profile - // (basically avoid recompiles) but historical defaults required - // that we always unwound. - panic_setting: if gctx.cli_unstable().panic_abort_tests { - PanicSetting::ReadProfile - } else { - PanicSetting::AlwaysUnwind - }, + panic_setting, root_compile_kind, artifact_target_for_features: None, } @@ -1130,8 +1139,14 @@ impl UnitFor { /// This is a special case for unit tests of a proc-macro. /// /// Proc-macro unit tests are forced to be run on the host. - pub fn new_host_test(gctx: &GlobalContext, root_compile_kind: CompileKind) -> UnitFor { - let mut unit_for = UnitFor::new_test(gctx, root_compile_kind); + /// + /// See [`UnitFor::new_test`] for more. + pub fn new_host_test( + gctx: &GlobalContext, + target: &Target, + root_compile_kind: CompileKind, + ) -> UnitFor { + let mut unit_for = UnitFor::new_test(gctx, target, root_compile_kind); unit_for.host = true; unit_for.host_features = true; unit_for diff --git a/src/cargo/ops/cargo_compile/unit_generator.rs b/src/cargo/ops/cargo_compile/unit_generator.rs index b2d86b7531c..5510154c649 100644 --- a/src/cargo/ops/cargo_compile/unit_generator.rs +++ b/src/cargo/ops/cargo_compile/unit_generator.rs @@ -148,7 +148,7 @@ impl<'a> UnitGenerator<'a, '_> { // // Forcing the lib to be compiled three times during `cargo // test` is probably also not desirable. - UnitFor::new_test(self.ws.gctx(), kind) + UnitFor::new_test(self.ws.gctx(), target, kind) } else if target.for_host() { // Proc macro / plugin should not have `panic` set. UnitFor::new_compiler(kind) diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index fe39e9e142b..fce2ba08336 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -428,22 +428,29 @@ fn profile_panic_test_with_custom_harness() { .file("benches/libtest.rs", "") .build(); + #[cfg(not(windows))] + let exit_code = 101; + #[cfg(windows)] + let exit_code = windows_sys::Win32::Foundation::STATUS_STACK_BUFFER_OVERRUN; + // panic abort on custom harness p.cargo("test --test custom --verbose") - .with_stderr_does_not_contain("[..]panic=abort[..]") + .with_stderr_contains("[RUNNING] `rustc --crate-name custom [..]-C panic=abort [..]") .with_stderr_contains("[..]thread '[..]' panicked at [..]") - .with_stderr_does_not_contain( + .with_stderr_contains("[..]abort![..]") + .with_stderr_contains( "[..]process didn't exit successfully: `[..]/target/debug/deps/custom-[..]", ) - .with_status(101) + .with_status(exit_code) .run(); p.cargo("bench --bench custom --verbose") - .with_stderr_does_not_contain("[..]panic=abort[..]") + .with_stderr_contains("[RUNNING] `rustc --crate-name custom [..]-C panic=abort [..]") .with_stderr_contains("[..]thread '[..]' panicked at [..]") - .with_stderr_does_not_contain( + .with_stderr_contains("[..]abort![..]") + .with_stderr_contains( "[..]process didn't exit successfully: `[..]/target/release/deps/custom-[..]", ) - .with_status(101) + .with_status(exit_code) .run(); // panic behaviour of libtest cannot be set as `abort` as of now. From b5d7be4077ee1693a2d0a9ecb1ce01964ee53f7d Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 May 2024 14:54:40 -0400 Subject: [PATCH 3/4] docs(profile): panic behavior can be specified for custom harness --- src/doc/src/reference/profiles.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/doc/src/reference/profiles.md b/src/doc/src/reference/profiles.md index fac84f8363e..ddba799e5ff 100644 --- a/src/doc/src/reference/profiles.md +++ b/src/doc/src/reference/profiles.md @@ -208,6 +208,10 @@ The `rustc` test harness currently requires `unwind` behavior. See the Additionally, when using the `abort` strategy and building a test, all of the dependencies will also be forced to build with the `unwind` strategy. +One exception is that when a test or benchmark target is driven by a custom +harness, panic setting will be applied since the custom harness may not have +the same limitation as the `rustc` test harness requires. + [`-C panic` flag]: ../../rustc/codegen-options/index.html#panic [`panic-abort-tests`]: unstable.md#panic-abort-tests From 36a965b124b2696dc6d0d7fcf1f0d6a315eefffe Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 May 2024 14:57:18 -0400 Subject: [PATCH 4/4] fix: remove warning "panic setting is ignored for test profile" --- src/cargo/util/toml/mod.rs | 5 ----- tests/testsuite/profiles.rs | 31 ------------------------------- tests/testsuite/test.rs | 2 +- 3 files changed, 1 insertion(+), 37 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 813868ac3ec..8d6db6d9eba 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2144,11 +2144,6 @@ pub fn validate_profile( "doc" => { warnings.push("profile `doc` is deprecated and has no effect".to_string()); } - "test" | "bench" => { - if root.panic.is_some() { - warnings.push(format!("`panic` setting is ignored for `{}` profile", name)) - } - } _ => {} } diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index fce2ba08336..badb7b5f8e9 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -365,37 +365,6 @@ must be a boolean (`true`/`false`) or a string (`\"thin\"`/`\"fat\"`/`\"off\"`) .run(); } -#[cargo_test] -fn profile_panic_test_bench() { - let p = project() - .file( - "Cargo.toml", - r#" - [package] - name = "foo" - version = "0.0.1" - edition = "2015" - - [profile.test] - panic = "abort" - - [profile.bench] - panic = "abort" - "#, - ) - .file("src/lib.rs", "") - .build(); - - p.cargo("build") - .with_stderr_contains( - "\ -[WARNING] `panic` setting is ignored for `bench` profile -[WARNING] `panic` setting is ignored for `test` profile -", - ) - .run(); -} - #[cargo_test] fn profile_panic_test_with_custom_harness() { // Custom harness can have `-C panic="…"` passed in. diff --git a/tests/testsuite/test.rs b/tests/testsuite/test.rs index a32cb6d0eba..1fbe5207249 100644 --- a/tests/testsuite/test.rs +++ b/tests/testsuite/test.rs @@ -4690,7 +4690,7 @@ fn panic_abort_only_test() { .build(); p.cargo("test -Z panic-abort-tests -v") - .with_stderr_contains("warning: `panic` setting is ignored for `test` profile") + .with_stderr_contains("[..]--crate-name foo [..]-C panic=abort[..]") .masquerade_as_nightly_cargo(&["panic-abort-tests"]) .run(); }