-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Bump bootstrap compiler to 1.68 #107297
Bump bootstrap compiler to 1.68 #107297
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
It looks like the rustfmt binary now dynamically links librustc_driver and libstd, which is causing breakage here. I'm not sure yet what introduced that bug, need to do a bisection to figure out. |
Rustfmt now links librustc_driver.so because of my PR #105609 to stop shipping rlibs in rustc-dev to reduce it's size. This means the only copy of the compiled code of rustc libraries is in librustc_driver.so and as such everything using them needs to link librustc_driver.so. I guess it would be possible to keep rustfmt statically link it by copying the respective rlibs to the sysroot only when building rustfmt, but not packaging it in rustc-dev. That may break out-of-tree rustfmt development though. |
Setting the LD_LIBRARY_PATH for rustfmt at runtime shouldn't be too hard, no? That seems better than special-casing rustfmt in bootstrap. |
For building LD_LIBRARY_PATH doesn't change a thing, we need the |
The problem is that at least today, we're not downloading the full nightly toolchain as part of bootstrap, just the rustfmt binary/package -- so there's nothing to point LD_LIBRARY_PATH to. |
Ok, can we change that? It seems reasonable to download rustc-dev for rustfmt's toolchain too. As a long term thing, it's never been clear to me why we don't just use beta rustfmt ... we can't use new language features until they're supported by the beta compiler anyway. I guess it matters for std, one more thing https://jyn.dev/2023/01/12/Bootstrapping-Rust-in-2023.html would help with. |
It shouldn't need
It would indeed be simpler if we got |
That would ~double the amount you need to download to work on rustc, which feels quite annoying. It may be the easiest short term fix though.
Rustfmt doesn't support RUSTC_BOOTSTRAP and we have unstable features via rustfmt.toml. The rustfmt team was reluctant to add support historically. |
I just had a thought: I will just skip bumping rustfmt, there's no hard need to do so. |
This comment was marked as resolved.
This comment was marked as resolved.
Or... not. I guess we're already using new syntax in std which the older rustfmt doesn't support. |
cc rust-lang/rustfmt#4884 for recent thread on RUSTC_BOOTSTRAP in rustfmt |
d054d5e
to
5f0e161
Compare
OK, updated with the logic to download rustc and rust-std components for the rustfmt toolchain (now placed in build/$triple/rustfmt). |
fwiw the rustfmt item is impacting downstream rustfmt users as well rust-lang/rustfmt#5675, and it also already has some minor impact on rustfmt developers https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/default.20sysroot.20for.20external.20tools/near/323956187 |
5f0e161
to
ac7b9c2
Compare
r=me with bjorn's comment addressed. |
ac7b9c2
to
9239808
Compare
This comment has been minimized.
This comment has been minimized.
9239808
to
652f79e
Compare
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
@bors r=pietroalbini rollup=iffy |
☀️ Test successful - checks-actions |
Finished benchmarking commit (dc1d9d5): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
let rustfmt_path = self.initial_rustc.with_file_name(exe("rustfmt", host)); | ||
let bin_root = self.out.join(host.triple).join("stage0"); | ||
let bin_root = self.out.join(host.triple).join("rustfmt"); | ||
let rustfmt_path = bin_root.join("bin").join(exe("rustfmt", host)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broke the rust-analyzer config we suggest in the dev-guide: https://rustc-dev-guide.rust-lang.org/building/suggested.html#configuring-rust-analyzer-for-rustc
"rust-analyzer.rustfmt.overrideCommand": [
"./build/host/stage0/bin/rustfmt",
"--edition=2021"
],
since now rustfmt is in build/host/rustfmt/bin/rustfmt
instead of build/host/stage0/bin/rustfmt
. What was the reason for this change? Is it possible to revert it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because we can't install multiple rustc
components to the same prefix path. Maybe we can symlink rustfmt
in the old place, although that won't work for Windows. Better would still be to use the same rustfmt
as the bootstrap compiler, if we can figure out the unstable features for that. (or stabilize what we need!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #107547 for more discussion.
As per rust-lang/rust#107297 (comment), the change broke the rust-analyzer config. Hence, changing the docs to match the new path
As per rust-lang/rust#107297 (comment), the change broke the rust-analyzer config. Hence, changing the docs to match the new path
Summary: Update to rustfmt 1.6.0-nightly (839e9a6 2023-07-02) The big ticket item here is that let-else is now supported! In fact, the suppport just landed two days ago (see [rust-lang/rustfmt#4914]) The unfortunate thing here though is that rustfmt is not statically linked anymore (see discussion in [rust-lang/rust#107297]). So we need all of librustc_driver and libstd - which makes our use case pretty big (~4-5MB to 50MB-100MB). We should explore building from source and statically linking, or using rustfmt from the toolchain. Both things that I don't want to deal with right now. [rust-lang/rustfmt#4914]: rust-lang/rustfmt#4914 [rust-lang/rust#107297]: rust-lang/rust#107297 Note to future updaters: To find out more-or-less what libs you need, you can use `objdump -p bin/rustfmt | grep NEEDED` on Linux for ELF bins, and `otool -L bin/rustfmt` on macOS for Mach-O bins. No idea what you do for Windows. Ran `tools/arcanist/lint/codemods/rustfmt-fbsource` to format the repo. Reviewed By: shayne-fletcher Differential Revision: D47203254 fbshipit-source-id: 6ffd3ce66c7f2b006d09505b93fed515ebc76902
Summary: Update to rustfmt 1.6.0-nightly (839e9a6 2023-07-02) The big ticket item here is that let-else is now supported! In fact, the suppport just landed two days ago (see [rust-lang/rustfmt#4914]) The unfortunate thing here though is that rustfmt is not statically linked anymore (see discussion in [rust-lang/rust#107297]). So we need all of librustc_driver and libstd - which makes our use case pretty big (~4-5MB to 50MB-100MB). We should explore building from source and statically linking, or using rustfmt from the toolchain. Both things that I don't want to deal with right now. [rust-lang/rustfmt#4914]: rust-lang/rustfmt#4914 [rust-lang/rust#107297]: rust-lang/rust#107297 Note to future updaters: To find out more-or-less what libs you need, you can use `objdump -p bin/rustfmt | grep NEEDED` on Linux for ELF bins, and `otool -L bin/rustfmt` on macOS for Mach-O bins. No idea what you do for Windows. Ran `tools/arcanist/lint/codemods/rustfmt-fbsource` to format the repo. Reviewed By: shayne-fletcher Differential Revision: D47203254 fbshipit-source-id: 6ffd3ce66c7f2b006d09505b93fed515ebc76902
Summary: Update to rustfmt 1.6.0-nightly (839e9a6 2023-07-02) The big ticket item here is that let-else is now supported! In fact, the suppport just landed two days ago (see [rust-lang/rustfmt#4914]) The unfortunate thing here though is that rustfmt is not statically linked anymore (see discussion in [rust-lang/rust#107297]). So we need all of librustc_driver and libstd - which makes our use case pretty big (~4-5MB to 50MB-100MB). We should explore building from source and statically linking, or using rustfmt from the toolchain. Both things that I don't want to deal with right now. [rust-lang/rustfmt#4914]: rust-lang/rustfmt#4914 [rust-lang/rust#107297]: rust-lang/rust#107297 Note to future updaters: To find out more-or-less what libs you need, you can use `objdump -p bin/rustfmt | grep NEEDED` on Linux for ELF bins, and `otool -L bin/rustfmt` on macOS for Mach-O bins. No idea what you do for Windows. Ran `tools/arcanist/lint/codemods/rustfmt-fbsource` to format the repo. Reviewed By: shayne-fletcher Differential Revision: D47203254 fbshipit-source-id: 6ffd3ce66c7f2b006d09505b93fed515ebc76902
This also changes our stage0.json to include the rustc component for the rustfmt pinned nightly toolchain, which is currently necessary due to rustfmt dynamically linking to that toolchain's librustc_driver and libstd.
r? @pietroalbini