Skip to content

refactor: separate "global" mode from CompileMode #15601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 29, 2025
Merged

refactor: separate "global" mode from CompileMode #15601

merged 2 commits into from
May 29, 2025

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented May 27, 2025 •
edited
Loading

What does this PR try to resolve?

This separates the concern of two different "mode".

  • UserIntent: focus on the overall goal of the build
  • CompileMode: the actual compile operation for each unit (I'd like to rename it to something else in the future, such as CompileAction)

This is a preparation of adding -Zno-link/-Zlink-only support,
which we'll have CompileMode::Link but that doesn't make sense to
show up in UserIntent.

How should we test and review this PR?

It should have no functional change.

Additional information

@rustbot
Copy link
Collaborator

rustbot commented May 27, 2025

r? @ehuss

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

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-layout Area: target output directory layout, naming, and organization A-lto Area: link-time optimization A-timings Area: timings Command-bench Command-build Command-check Command-doc Command-fetch Command-fix Command-install Command-package Command-run Command-rustc Command-rustdoc Command-test S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2025
@weihanglo weihanglo removed Command-test Command-run Command-install Command-check Command-bench Command-build Command-fetch Command-package Command-rustc Command-rustdoc A-build-scripts Area: build.rs scripts Command-fix A-timings Area: timings A-cli Area: Command-line interface, option parsing, etc. A-cfg-expr Area: Platform cfg expressions A-layout Area: target output directory layout, naming, and organization A-lto Area: link-time optimization labels May 27, 2025
ehuss
ehuss approved these changes May 28, 2025
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ehuss ehuss added this pull request to the merge queue May 28, 2025
Merged via the queue into rust-lang:master with commit bffece8 May 29, 2025
23 checks passed
bjorn3
bjorn3 reviewed May 29, 2025
View reviewed changes
src/cargo/core/compiler/build_config.rs
pub enum UserIntent {
/// Build benchmark binaries, e.g., `cargo bench`
Bench,
/// Build binaries and libraray, e.g., `cargo run`, `cargo install`, `cargo build`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*library

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually should be plural 😬.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#15608

bjorn3
bjorn3 reviewed May 29, 2025
View reviewed changes
src/cargo/core/compiler/build_config.rs
/// A target that will be documented with `rustdoc`.

/// Document with `rustdoc`.
///
/// If `deps` is true, then it will also document all dependencies.
/// if `json` is true, the documentation output is in json format.
Doc { deps: bool, json: bool },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the deps field on CompileMode too and not just on UserIntent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you need it to determine whether to include deps for documenting, during computing the unit graph. I haven't found a good way to remove that.

cargo/src/cargo/core/compiler/unit_dependencies.rs

Lines 632 to 645 in bffece8

if let CompileMode::Doc { deps: true, .. } = unit.mode {
// Document this lib as well.
let doc_unit_dep = new_unit_dep(
state,
unit,
dep_pkg,
dep_lib,
dep_unit_for,
unit.kind.for_target(dep_lib),
unit.mode,
IS_NO_ARTIFACT_DEP,
)?;
ret.push(doc_unit_dep);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured you would have access to the UserIntent when deciding if a crate should be documented, but I guess not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that. You're right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went ahead and removed the deps field as well. Thanks for the good catch!

@weihanglo weihanglo deleted the compilemode branch May 29, 2025 12:30
@weihanglo weihanglo mentioned this pull request May 29, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 30, 2025
@ehuss
refactor: cleanup for CompileMode (#15608)
e165863 
### 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.
@weihanglo weihanglo mentioned this pull request May 30, 2025
Merged
bors added a commit to rust-lang/rust that referenced this pull request May 31, 2025
@bors
Auto merge of #141796 - weihanglo:update-cargo, r=weihanglo
7a7bcbb 
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
@rustbot rustbot added this to the 1.89.0 milestone May 31, 2025
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request May 31, 2025
@bors
Auto merge of #141796 - weihanglo:update-cargo, r=weihanglo
415e792 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@ehuss ehuss ehuss approved these changes

Assignees

@ehuss ehuss

Labels
A-build-execution Area: anything dealing with executing the compiler 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.
Projects
None yet
Milestone
1.89.0
Development

Successfully merging this pull request may close these issues.
4 participants
@weihanglo @rustbot @ehuss @bjorn3