Skip to content

add tool::CargoClippy and tool::Cargofmt binary to target sysroot #137541

New issue

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

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

Already on GitHub? # to your account

Merged
merged 2 commits into from
Mar 9, 2025

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Feb 24, 2025

When running x build clippy, we expect stage1-tool-bin/cargo-clippy and stage2/bin/cargo-clippy to be the same, but they aren't. This happens because tool::CargoClippy doesn't place its binary in the stage2 directory. As a result, stage1-tool-bin/cargo-clippy comes from tool::CargoClippy, while stage2/bin/cargo-clippy comes from tool::Cargo. Same applies for tool::Cargofmt.

This PR fixes the issue by adding tool::CargoClippy and tool::Cargofmt binaries to the expected sysroot and makes sure both directories share the same binary.

To test this, run x build --stage 2 compiler clippy rustfmt, link the stage2 sysroot with rustup, and then call cargo +stage2 fmt and cargo +stage2 clippy on any rust project (it wouldn't work without this PR).

@onur-ozkan
Copy link
Member Author

r? bootstrap

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Feb 24, 2025
@onur-ozkan onur-ozkan changed the title add tool::CargoClippy binary to target sysroot add tool::CargoClippy and tool::Cargofmt binary to target sysroot Feb 24, 2025
@onur-ozkan
Copy link
Member Author

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 24, 2025
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2025
jieyouxu

This comment was marked as outdated.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

I tried the changes locally and can confirm I can reproduce that cargo-fmt and cargo-clippy are present in stage2 sysroot, and that they are functional.

Thanks for the fix!

@jieyouxu
Copy link
Member

How did I end up with two comments 🤔 Anyway

@bors r+ rollup=never (tool build changes)

@bors
Copy link
Collaborator

bors commented Feb 25, 2025

📌 Commit f677bab has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2025
@Kobzol
Copy link
Contributor

Kobzol commented Feb 25, 2025

Interesting, did it cause the following behavior? When I use nightly and look at the version of the two binaries, they seem to be different:

~/.ru/to/nightly/bin$ ./cargo-fmt --version
rustfmt 1.8.0-stable (4d91de4e48 2025-02-17)
~/.ru/to/nightly/bin$ ./rustfmt --version
rustfmt 1.8.0-nightly (617aad8c2e 2025-02-24)

@jieyouxu
Copy link
Member

Hm, should double check.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 26, 2025
@jieyouxu
Copy link
Member

Wait no, not the changes in this PR (that I'm fairly sure is correct).

@bors r+

@Kobzol
Copy link
Contributor

Kobzol commented Feb 26, 2025

To clarify, this was happening with the previous nightly before this PR, I was just wondering whether 1) it is something that should be fixed 2) this PR fixes it :)

@bors
Copy link
Collaborator

bors commented Feb 26, 2025

📌 Commit f677bab has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2025
@jieyouxu
Copy link
Member

To clarify, this was happening with the previous nightly before this PR, I was just wondering whether 1) it is something that should be fixed 2) this PR fixes it :)

Can you file this separately to track it (in case this PR doesn't fix it)? I'm not at PC atm.

@Kobzol
Copy link
Contributor

Kobzol commented Feb 26, 2025

#137666

Noratrieb added a commit to Noratrieb/rust that referenced this pull request Mar 6, 2025
…=jieyouxu

add `tool::CargoClippy` and `tool::Cargofmt` binary to target sysroot

When running `x build clippy`, we expect `stage1-tool-bin/cargo-clippy` and `stage2/bin/cargo-clippy` to be the same, but they aren't. This happens because `tool::CargoClippy` doesn't place its binary in the `stage2` directory. As a result, `stage1-tool-bin/cargo-clippy` comes from `tool::CargoClippy`, while `stage2/bin/cargo-clippy` comes from `tool::Cargo`. Same applies for `tool::Cargofmt`.

This PR fixes the issue by adding `tool::CargoClippy` and ``tool::Cargofmt`` binaries to the expected sysroot and makes sure both directories share the same binary.

To test this, run `x build --stage 2 compiler clippy rustfmt`, link the stage2 sysroot with rustup, and then call `cargo +stage2 fmt` and `cargo +stage2 clippy` on any rust project (it wouldn't work without this PR).
@bors
Copy link
Collaborator

bors commented Mar 9, 2025

⌛ Testing commit f677bab with merge a96fa31...

@bors
Copy link
Collaborator

bors commented Mar 9, 2025

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing a96fa31 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 9, 2025
@bors bors merged commit a96fa31 into rust-lang:master Mar 9, 2025
7 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 9, 2025
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

gh pr comment ${HEAD_PR} -F output.log
shell: /usr/bin/bash -e {0}
##[endgroup]
fatal: ambiguous argument 'HEAD^1': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
##[error]Process completed with exit code 128.

@onur-ozkan onur-ozkan deleted the fix-cargo-clippy-bin branch March 9, 2025 08:08
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a96fa31): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -5.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-5.7% [-5.7%, -5.7%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -5.7% [-5.7%, -5.7%] 1

Cycles

Results (secondary 2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 764.414s -> 765.189s (0.10%)
Artifact size: 362.00 MiB -> 362.04 MiB (0.01%)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants