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.

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

bootstrap: show diagnostics relative to rustc src dir #132390

Merged
merged 3 commits into from
Dec 1, 2024
Merged

bootstrap: show diagnostics relative to rustc src dir #132390

merged 3 commits into from
Dec 1, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 31, 2024 •
edited
Loading

Fixes #128726

Depends on rust-lang/cargo#14752 propagating to bootstrap cargo

@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2024

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 31, 2024
@RalfJung RalfJung mentioned this pull request Oct 31, 2024
Closed
@jieyouxu jieyouxu added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2024
@RalfJung RalfJung mentioned this pull request Oct 31, 2024
Merged
@jieyouxu jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 31, 2024
@jieyouxu
Copy link
Member

@RalfJung by "Depends on rust-lang/cargo#14752" do you mean this PR is blocked on the cargo PR being merged and synced first?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

RalfJung commented Oct 31, 2024 •
edited
Loading

@RalfJung by "Depends on rust-lang/cargo#14752" do you mean this PR is blocked on the cargo PR being merged and synced first?

That PR needs to propagate to bootstrap cargo.

@bjorn3 bjorn3 mentioned this pull request Oct 31, 2024
Closed
bors added a commit to rust-lang/cargo that referenced this pull request Oct 31, 2024
@bors
Auto merge of #14752 - RalfJung:rustc-root-path, r=epage
40d804b 
add unstable -Zroot-dir flag to configure the path from which rustc should be invoked

This implements the proposal described [here](#9887 (comment)): we add a new flag, for now called `-Zroot-dir`, that configures the directory relative to which rustc is given the crate root filenames to build. (Files outside this directory are passed absolutely.)

This is necessary to be able to fix (no github don't close that issue yet) rust-lang/rust#128726: in multi-workspace repositories that use scripts to manage a whole bunch of cargo invocations, currently the output cargo+rustc produce is often hard or even impossible to interpret for both human and machine consumption. This is because directories in the output are always relative to the workspace root, but when cargo is invoked many times for different workspaces, it is quite unclear what the workspace root is for the invocation that failed.

So I suggest we should have a new flag that the build script in such a repo can set to the consistent "root dir" that the user would recognize as such (e.g., the root of the rustc source tree), and all paths emitted by cargo and rustc should be relative to that directory.

I don't know all the places that cargo itself emits paths (if any), but this PR changes the way we invoke rustc to honor the new flag, so all paths emitted by rustc will be relative to the `-Zroot-dir`.

See rust-lang/rust#132390 for the changes needed in rustc bootstrap to wire this up; together, that suffices to finally properly show errors in RA for all parts of the rustc src tree. :)
@RalfJung RalfJung added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2024
@albertlarsan68 albertlarsan68 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 2, 2024
@albertlarsan68
Copy link
Member

@RalfJung the cargo side has been synced, r=me when CI is green after a rebase :)

@bjorn3
Copy link
Member

bjorn3 commented Nov 2, 2024

We need to wait until the cargo PR is part of the bootstrap toolchain, right?

@RalfJung
Copy link
Member Author

RalfJung commented Nov 2, 2024 via email

@jieyouxu jieyouxu added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 2, 2024
@RalfJung RalfJung force-pushed the diagnostics-root-dir branch from 367dffc to 846d03c Compare November 2, 2024 15:59
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

The next beta branches on November 22, so I guess the bootstrap bump will be shortly after that.

This was referenced Nov 18, 2024
Merged
Closed
@bors
Copy link
Contributor

bors commented Nov 30, 2024

📌 Commit c4cab8a has been approved by albertlarsan68

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 30, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 30, 2024
@bors
Copy link
Contributor

bors commented Nov 30, 2024

⌛ Testing commit c4cab8a with merge 938866d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 30, 2024
@bors
Auto merge of rust-lang#132390 - RalfJung:diagnostics-root-dir, r=alb…
938866d 
…ertlarsan68

bootstrap: show diagnostics relative to rustc src dir

Fixes rust-lang#128726

Depends on rust-lang/cargo#14752 propagating to bootstrap cargo
@cuviper
Copy link
Member

cuviper commented Dec 1, 2024

Looks like this will change the file paths printed in panics that point at the standard library, by including the leading library/. @rust-lang/libs that's an unstable implementation detail we are allowed to change, I assume?

I think that's fine, especially since the rust-src component does ship with that library/ part too.

A downside is that this adds a little bit of bloat, making every standard library path in the binary 8 bytes longer. It's still not much even if all ~1500 files are referenced, but I wonder if some size-sensitive embedded folks will notice.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Dec 1, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 1, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Dec 1, 2024

@rust-lang/cargo seems like there's again a problem with that cargo path logic...

@RalfJung
Copy link
Member Author

RalfJung commented Dec 1, 2024

I found a different way to make this work, by adjusting the patch that was added in #133533.
@rust-lang/bootstrap could you take a look?

Note that if snapbox ever removes their CARGO_RUSTC_CURRENT_DIR case (which arguably they should, as that env var no longer officially exists), this will break again. The proper solution for this is rust-lang/libs-team#478. But I hope since @epage is snapbox maintainer, they will keep supporting CARGO_RUSTC_CURRENT_DIR until cargo (including via ./x test cargo) doesn't need it any more. :)

@RalfJung RalfJung mentioned this pull request Dec 1, 2024
Open
@RalfJung RalfJung force-pushed the diagnostics-root-dir branch from d05bc03 to dd2ac08 Compare December 1, 2024 10:21
@weihanglo
Copy link
Member

The adjustment looks good to me.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 1, 2024

@bors r=albertlarsan68,weihanglo

@bors
Copy link
Contributor

bors commented Dec 1, 2024

📌 Commit dd2ac08 has been approved by albertlarsan68,weihanglo

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 Dec 1, 2024
@bors
Copy link
Contributor

bors commented Dec 1, 2024

⌛ Testing commit dd2ac08 with merge ca4e54f...

@bors
Copy link
Contributor

bors commented Dec 1, 2024

☀️ Test successful - checks-actions
Approved by: albertlarsan68,weihanglo
Pushing ca4e54f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 1, 2024
@bors bors merged commit ca4e54f into rust-lang:master Dec 1, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 1, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ca4e54f): 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 (secondary -0.6%)

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.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.1% [-3.1%, -3.1%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

Results (primary 0.0%, secondary 0.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.0% [0.0%, 0.1%] 12
Regressions ❌
(secondary)		 0.1% [0.1%, 0.1%] 35
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.0% [0.0%, 0.1%] 12

Bootstrap: 767.632s -> 768.479s (0.11%)
Artifact size: 332.19 MiB -> 332.12 MiB (-0.02%)

@RalfJung RalfJung deleted the diagnostics-root-dir branch December 2, 2024 16:57
@RalfJung RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 5, 2024
@rustbot rustbot mentioned this pull request Dec 5, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@albertlarsan68 albertlarsan68

Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. 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
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.

Rust-analyzer + rustc repo: build failures in the standard library are not shown in the editor any more
10 participants
@RalfJung @rustbot @jieyouxu @rust-log-analyzer @albertlarsan68 @bjorn3 @bors @cuviper @weihanglo @rust-timer