-
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
Rename sysroot directories to be more clear #103286
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon. Please see the contribution instructions for more information. |
@chetankokil these changes don't look correct. Is there something about the instructions in #101961 (comment) you found confusing? |
apologies , read the comments in wrong manner. Made the changes now as per the comments. hope this looks good.' |
@rustbot unclaim |
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.
Thanks! This looks a lot better. Can you also add a line to src/bootstrap/CHANGELOG.md explaining that this will break rustup toolchain link
toolchains and giving upgrade instructions? Please also bump MAJOR_VERSION
in src/bootstrap/bin/bootstrap.rs
and the changelog.
@chetankokil why did you close this pr? |
This comment has been minimized.
This comment has been minimized.
sir the build is failing. not sure how to resolve the |
This comment has been minimized.
This comment has been minimized.
@chetankokil I think you also need to update Line 1251 in 3602282
Line 1596 in 3602282
and Line 1603 in 3602282
|
…ded changes to CHANGELOG.md
Done. And added to the PR |
Hi @chetankokil , the code here looks good, you just need to make a PR to the dev guide like I suggested in #103286 (comment) . Once you do that we can merge this change :) |
Co-authored-by: Joshua Nelson <github@jyn.dev>
The directory names for the stages are changing in version 3 of the boostrapping (see rust-lang/rust#103286). We are checking the version now to create the correct dummy directories.
☔ The latest upstream changes (presumably #104188) made this pull request unmergeable. Please resolve the merge conflicts. |
Ok, docs PR is up: rust-lang/rustc-dev-guide#1510 @Mark-Simulacrum do you have opinions on this before I merge it? I think it might be useful to make a blog post, since this is going to break a bunch of workflows? I did bump the major version but I worry people won't actually read the changelog. |
Yes, I think something is in order messaging wise. I'll take a look at my next review cycle, probably this weekend. |
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
Ok, leaving a couple comments here:
Broadly speaking I am fine with this change otherwise but can't say I'm particularly enthusiastic about this meaningfully increasing legibility. They seem a little easier to refer to at the very least though. |
@chetankokil can you please work on the things that Mark asked for? You can make a blog post by making a PR to https://github.com/rust-lang/blog.rust-lang.org. |
@jyn514 i tried merging and its not working anymore. looks like lots of things have changed. i need to regroup myself and see what changes to incorporate. |
@chetankokil |
there is no update from my side. you can close the PR.
…On Sun, Jan 29, 2023 at 1:01 AM John Simon ***@***.***> wrote:
@chetankokil <https://github.com/chetankokil>
Ping from triage - can you please post your status on this? Thank you.
—
Reply to this email directly, view it on GitHub
<#103286 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4MZYPMUR25S7FCLSBPW7DWUYIVTANCNFSM6AAAAAARJXU6SI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sorry this didn't work out. Thank you for working on the PR. |
Fixes #101961