Skip to content
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.

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

fix(cargo-fix): dont fix into standard library #13792

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

Protect standard library source from being fixed by cargo fix.

This is a bug in rustc, which it shouldn't have given any fix suggestion to the standard library.
However, I believe Cargo should have a layer of protection regardless it's a bug in rustc or not.

See #9857 (comment)

How should we test and review this PR?

This first commit shows the problematic behavior. The second fixes it.

The alternative idea has been brought up: only paths under workspace root are fixable.
However, it might encounter some corner cases:

  • #[path] attribute pointing outside a workspace.
  • Path dependencies that are not inside the same workspace.

I took this conservative approach that Cargo just stop writing into whatever pointing to sysroot.

r? ehuss

The suggestion is slightly tweaked since the original
diagnostic was rejected due to multiple files were involved.

For the original diagnostic JSON, see
rust-lang/rust#88514 (comment)
This takes a more aggressive approach by stop any write into sysroot.
@rustbot rustbot added Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2024
@ehuss
Copy link
Contributor

ehuss commented Apr 30, 2024

Thanks! I still hope we can get this fixed in rustc (rust-lang/rust#88514) so it doesn't provide external suggestions, but for now this seems good.

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 30, 2024

📌 Commit 0810627 has been approved by ehuss

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 Apr 30, 2024
@bors
Copy link
Collaborator

bors commented Apr 30, 2024

⌛ Testing commit 0810627 with merge 6087566...

@bors
Copy link
Collaborator

bors commented Apr 30, 2024

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 6087566 to master...

@bors bors merged commit 6087566 into rust-lang:master Apr 30, 2024
21 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request May 1, 2024
Update cargo

15 commits in b60a1555155111e962018007a6d0ef85207db463..6087566b3fa73bfda29702632493e938b12d19e5
2024-04-26 16:37:29 +0000 to 2024-04-30 20:45:20 +0000
- fix(cargo-fix): dont fix into standard library (rust-lang/cargo#13792)
- refactor: Move diagnostic printing to Shell (rust-lang/cargo#13813)
- Populate git information when building Cargo from Rust's source tarball (rust-lang/cargo#13832)
- docs: fix several typos found by `typos-cli` (rust-lang/cargo#13831)
- fix(alias): Aliases without subcommands should not panic (rust-lang/cargo#13819)
- fix(toml): Improve granularity of traces (rust-lang/cargo#13830)
- fix(toml): Warn, rather than fail publish, if a target is excluded (rust-lang/cargo#13713)
- test(cargo-lints): Add a test to ensure cap-lints works (rust-lang/cargo#13829)
- fix(toml)!: Remove support for inheriting badges (rust-lang/cargo#13788)
- chore(ci): Don't check `cargo` against beta channel (rust-lang/cargo#13827)
- Fix target entry in .gitignore (rust-lang/cargo#13817)
- Bump to 0.81.0; update changelog (rust-lang/cargo#13823)
- Add failing test: artifact_dep_target_specified (rust-lang/cargo#13816)
- fix(cargo-lints): Don't always inherit workspace lints (rust-lang/cargo#13812)
- Update SleepTraker returns_in_order unit test (rust-lang/cargo#13811)

r? ghost
@rustbot rustbot added this to the 1.80.0 milestone May 1, 2024
@RalfJung
Copy link
Member

RalfJung commented May 9, 2024

I took this conservative approach that Cargo just stop writing into whatever pointing to sysroot.

So -- it may still be writing into the crates downloaded from the registry? That seems just as problematic as changing std?

@weihanglo weihanglo deleted the fix-in-rust-src branch May 9, 2024 20:44
@weihanglo
Copy link
Member Author

@RalfJung

Not really. We built a protection for registry caches since 1.57 #9938.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-fix S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants