refactor: cleanup for `CompileMode` #15608

weihanglo · 2025-05-29T12:39:24Z

What does this PR try to resolve?

Some more cleanup after #15601

See each commit message for details.

How should we test and review this PR?

Help review whether it has really no behavior change.

rustbot · 2025-05-29T12:39:29Z

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

To justify why it won't affect any existing behavior: * For the default set of Cargo targets, test/bench targets would only be selected by `cargo test`, whose UserIntent will turn into `CompileMode::Test`, but never `CompileMode::Build`. * For selective Cargo targets, i.e., `--tests`, or `--test`, Their `CompileMode` would either be `Check { test }` or `Test`. They will never be `Build`. See https://github.com/rust-lang/cargo/blob/bffece899e9/src/cargo/ops/cargo_compile/unit_generator.rs#L450-L465 According to these facts, there won't be a case that bench/test targets get assigned to `CompileMode::Build`.

They are globally determined at Cargo invocation level, and can all be derived from `UserIntent`.

`cargo test` will implicitly build examples as examples binaries (without --test) by default, when no target selection flags provided. We don't have test exercising this, so add one.

ehuss · 2025-05-30T13:56:08Z

src/cargo/ops/cargo_compile/unit_generator.rs

+    /// * Like other test targets, it shouldn't respect profile panic settings.
+    ///   This is for backward compatibility. Whether it is by design, not sure.


I believe this is intentional. I think the comment at

cargo/src/cargo/ops/cargo_compile/unit_generator.rs

Lines 106 to 125 in 2a8a85f

let unit_for = if is_test {

// NOTE: the `UnitFor` here is subtle. If you have a profile

// with `panic` set, the `panic` flag is cleared for

// tests/benchmarks and their dependencies. If this

// was `normal`, then the lib would get compiled three

// times (once with panic, once without, and once with

// `--test`).

//

// This would cause a problem for doc tests, which would fail

// because `rustdoc` would attempt to link with both libraries

// at the same time. Also, it's probably not important (or

// even desirable?) for rustdoc to link with a lib with

// `panic` set.

//

// As a consequence, Examples and Binaries get compiled

// without `panic` set. This probably isn't a bad deal.

//

// Forcing the lib to be compiled three times during `cargo

// test` is probably also not desirable.

UnitFor::new_test(self.ws.gctx(), kind)

explains the reasoning. We want to avoid compiling the library multiple times. Does that make sense?

Overall I'm not so certain about this commit. Instead of checking in one place (at a low level), it pushes it to a higher level that requires checking in multiple places. There's also the risk that that could get missed in the future if there are other changes that would require checking for it.

Can you say more about what made you feel uncomfortable about checking it at the low level?

Nah. Just felt it better to have one function finalizing everything, and the other do the conversion. But yeah it doesn't really make sense doing it in two places. Also, new_units is not a public function, and a regression test is added, so should be fine without this commit.

Removed!

ehuss

Thanks!

Update cargo 12 commits in 68db37499f2de8acef704c73d9031be6fbcbaee4..64a12460708cf146e16cc61f28aba5dc2463bbb4 2025-05-22 14:27:15 +0000 to 2025-05-30 18:25:08 +0000 - chore: remove HTML comments and inline guide (rust-lang/cargo#15613) - Add .git-blame-ignore-revs (rust-lang/cargo#15612) - refactor: cleanup for `CompileMode` (rust-lang/cargo#15608) - refactor: separate "global" mode from CompileMode (rust-lang/cargo#15601) - fix(doc): pass `toolchain-shared-resources` to get doc styled (rust-lang/cargo#15605) - fix(embedded): Resolve multiple bugs in frontmatter parser (rust-lang/cargo#15573) - chore: Upgrade schemars (rust-lang/cargo#15602) - Update gix & socket2 (rust-lang/cargo#15600) - Add `-Zfix-edition` (rust-lang/cargo#15596) - chore(toml): disable `toml`'s default features, unless necessary (rust-lang/cargo#15598) - docs(README): fix the link to the changelog in the Cargo book (rust-lang/cargo#15597) - Add the future edition (rust-lang/cargo#15595) r? ghost

docs: fix typo

bef07b2

rustbot assigned ehuss May 29, 2025

rustbot added A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2025

weihanglo force-pushed the compile-mode branch from e241099 to 5fb96e8 Compare May 29, 2025 12:51

weihanglo mentioned this pull request May 29, 2025

refactor: separate "global" mode from CompileMode #15601

Merged

refactor: remove fields in CompileMode::Doc

d0cbbac

They are globally determined at Cargo invocation level, and can all be derived from `UserIntent`.

rustbot added A-build-execution Area: anything dealing with executing the compiler A-layout Area: target output directory layout, naming, and organization labels May 29, 2025

weihanglo changed the title ~~refactor: remove unreachable match arms for tests/benches~~ refactor: cleanup for CompileMode May 29, 2025

test(test): ensure panic=abort not added to default example inclusion

8a388b0

`cargo test` will implicitly build examples as examples binaries (without --test) by default, when no target selection flags provided. We don't have test exercising this, so add one.

ehuss reviewed May 30, 2025

View reviewed changes

weihanglo force-pushed the compile-mode branch from 2a8a85f to 8a388b0 Compare May 30, 2025 14:04

ehuss approved these changes May 30, 2025

View reviewed changes

ehuss added this pull request to the merge queue May 30, 2025

Merged via the queue into rust-lang:master with commit e165863 May 30, 2025
23 checks passed

weihanglo deleted the compile-mode branch May 30, 2025 16:53

weihanglo mentioned this pull request May 30, 2025

Update cargo rust-lang/rust#141796

Merged

rustbot added this to the 1.89.0 milestone May 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: cleanup for `CompileMode` #15608

refactor: cleanup for `CompileMode` #15608

Uh oh!

weihanglo commented May 29, 2025 •

edited

Loading

Uh oh!

rustbot commented May 29, 2025

Uh oh!

ehuss May 30, 2025

Uh oh!

weihanglo May 30, 2025

Uh oh!

ehuss left a comment

Uh oh!

Uh oh!

Uh oh!

		/// * Like other test targets, it shouldn't respect profile panic settings.
		/// This is for backward compatibility. Whether it is by design, not sure.

	let unit_for = if is_test {
	// NOTE: the `UnitFor` here is subtle. If you have a profile
	// with `panic` set, the `panic` flag is cleared for
	// tests/benchmarks and their dependencies. If this
	// was `normal`, then the lib would get compiled three
	// times (once with panic, once without, and once with
	// `--test`).
	//
	// This would cause a problem for doc tests, which would fail
	// because `rustdoc` would attempt to link with both libraries
	// at the same time. Also, it's probably not important (or
	// even desirable?) for rustdoc to link with a lib with
	// `panic` set.
	//
	// As a consequence, Examples and Binaries get compiled
	// without `panic` set. This probably isn't a bad deal.
	//
	// Forcing the lib to be compiled three times during `cargo
	// test` is probably also not desirable.
	UnitFor::new_test(self.ws.gctx(), kind)

refactor: cleanup for CompileMode #15608

refactor: cleanup for CompileMode #15608

Uh oh!

Conversation

weihanglo commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How should we test and review this PR?

Uh oh!

rustbot commented May 29, 2025

Uh oh!

ehuss May 30, 2025

Choose a reason for hiding this comment

Uh oh!

weihanglo May 30, 2025

Choose a reason for hiding this comment

Uh oh!

ehuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

refactor: cleanup for `CompileMode` #15608

refactor: cleanup for `CompileMode` #15608

weihanglo commented May 29, 2025 •

edited

Loading