-
Notifications
You must be signed in to change notification settings - Fork 515
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
Getting started guide #731
Conversation
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.
Overall this looks really good! 🎉
This looks like a great start Mark!. |
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 looks really helpful! I mostly made a few typo fixes :)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Camelid <37223377+camelid@users.noreply.github.com>
So I've been thinking a bit about what command to list. I'm thinking of changing how these sections are currently written as follows:
The reason is that I think it would be good for people to be prepared for the shortcuts to fail sometimes. I think it would be really frustrating to read the "Getting Started" guide, do what it says, and get a weird error. Moreover, I always think it's good for people to get a correct high-level understanding of what's going on. Thoughts? |
Although if people just read the first (correct) part, and skip the rest, then they'll think that rustc takes forever to compile every time they make a change and will get frustrated for a different reason. |
@camelid True, but I think we can format the guide well to draw attention to it. Also, what you described today seems to already be the case 😛 |
cc @rust-lang/rustdoc |
Co-authored-by: Camelid <37223377+camelid@users.noreply.github.com> Co-authored-by: Joshua Nelson <joshua@yottadb.com> Co-authored-by: Phil Hansch <dev@phansch.net>
Ok, I think this is ready. Thanks to all who left comments and to @jyn514 for filling in those missing commands. I would like to merge this and iterate in followup PRs. |
Co-authored-by: Camelid <37223377+camelid@users.noreply.github.com>
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.
My review part 1 :)
src/getting-started.md
Outdated
[boot]: ./building/bootstrapping.md | ||
|
||
We have a special tool `./x.py` that drives this process. It is used for | ||
compiling the compiler, the standard libraries, and `rustdoc`. It is also used |
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 I've always heard of the "standard library" as a single piece of code, but I might be wrong and this is a nitpick, really 😄
I need those nits to make a review. Because there isn't much to say otherwise...
compiling the compiler, the standard libraries, and `rustdoc`. It is also used | |
compiling the compiler, the standard library, and `rustdoc`. It is also used |
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.
Mostly I said libraries because there is std, core, alloc, proc_macro, test, etc... not sure what else to call them.
However, if you are changing certain parts of the compiler, this may lead to | ||
weird errors. Feel free to ask on [zulip][z] if you are running into issues. | ||
|
||
This runs a ton of tests and takes a long time to complete. If you are |
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 am unsure that the tests take all available CPU cores to run, and in my experience I was under the impression that the tests ran faster if I passed the --jobs
flag. I am sure of nothing though, I should probably dive into the sources for the bootstrap to find out.
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.
TMK, compiletest will use all available CPUs and run as many parallel tasks as possible. I might be wrong though. I just remember that on my laptop compiletest will max out CPU.
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.
the tests ran faster if I passed the --jobs flag
This depends on what you have codegen-units
set to in config.toml
. I have it set to codegen-units = 0
but I think the default might be 1.
Co-authored-by: LeSeulArtichaut <leseulartichaut@gmail.com>
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 is great. I just left some notes from a read, but I wouldn't block on these suggestions, and I'm not even sure if I agree with them. I think one thing that is clear from reading this is how many knobs we have. Might be good inspiration to look for ways to simplify things! (Not that I have any particular idea in that respect, beyond pushing us to align with a more standard cargo-based workflow)
``` | ||
|
||
Then, edit `config.toml`. You may want to search for, uncomment, and update | ||
the following settings: |
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 we should consider having a (minimal) config.toml.dev
or something that has the recommended settings.
That said, I do not recommend debug = true
-- but maybe my advice is out of date. I do recommend debug-assertions=true
, as well as ccache
for LLVM, though I've never tried using the system LLVM, that may be better indeed.
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.
Without debug = true
, you won't see any debug!
logging. @eddyb recommended debug = true
and debuginfo = 1
a while back, maybe we could change debuginfo to default to 1 instead of 2 when debug
is set?
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've started this hackmd to collect various suggestions that have come up: https://hackmd.io/@ux-jKBcgRTSHsH042VF3BA/rkXEozKTU/edit
./x.py test | ||
``` | ||
|
||
For most contributions, you only need to build stage 1, which saves a lot of time: |
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 feel like if we think this is what people should do most of the time, we should suggest it first, maybe?
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.
Hmm I also think it'd be good to integrate the x.py check
.
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 that my own workflow at this point is basically:
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.
- Use
x.py check
while editing; try whenever possible to structure my changes into things that the compiler can fully enforce, and make commits as I go. This also helps with reviewing. - Run
x.py build
andx.py test --bless
when necessary.
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.
Anyway I don't know, I'm not sure what specific suggestion I have, but I think we could convey this information better. I was toying with the idea of a flowchart (see this mermaid live editor example) but I think it's maybe not that effective.
I guess I feel like it'd be nice to present the information in a "summary form", maybe something like this instead?
Command | When to use it |
---|---|
x.py check |
Quick check to see if things compile; rust-analyzer can run this automatically for you |
x.py build --stage 1 |
Build just the 1st stage of the compiler; this is faster than building stage 2 and usually good enough |
x.py build --stage 1 --keep-stage 1 |
Build the 1st stage of the compiler and skips rebuilding the library; this is useful after you've done an ordinary stage1 build to skip compilation time, but it can cause weird problems. (Just do a regular build to resolve.) |
x.py test --stage 1 |
Run the test suite using the stage1 compiler |
x.py test --stage 1 --bless |
... |
...and then give more details below?
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.
So, I definitely agree that I think we should make the "default" commands the ones that most people need rather than what release does (e.g. ./x.py build
would do a stage-1 incremental build), but it also seems odd to have the default command be technically unsound...
I'm not really sure what to make of this situation...
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.
Oh, also, I forgot to mention --bless
...
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 vote for the most common commands first, with a disclaimer that they may not work and you should ask on Zulip.
Since it’s a getting started guide, I think the command that beginners will use more should be at the top.
Maybe mention that after a major rust release, you should run the sound command since now the older part was compiled with a different compiler? At least, I think I needed to do that after 1.44
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.
Maybe say not to worry about using bless often since you can just compare the git diff later? At first I thought it was better to edit stderr manually, until I realized I could just diff later
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 added Niko's table to the top of the section.
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.
My review, part 2 :D Yet another batch of nitpicks!
Co-authored-by: LeSeulArtichaut <leseulartichaut@gmail.com>
Ok, thanks again everyone! I'm going to go ahead and merge this. It looks like there are still a few unresolved questions. Please feel free to continue discussion here or open a new issue ❤️ |
| `x.py test --stage 1 --keep-stage 1` | Run the test suite using the stage1 compiler (subsequent builds) | | ||
| `x.py test --stage 1 --bless [--keep-stage 1]` | Run the test suite using the stage1 compiler _and_ update expected test output. | | ||
| `x.py build` | Do a full 2-stage build. You almost never want to do this. | | ||
| `x.py test` | Do a full 2-stage build and run all tests. You almost never want to do this. | |
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.
The only time I've had this come up was for rust-lang/rust#68692 (comment), where something failed in stage 2 libtest and nowhere else. That was definitely an edge case though.
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.
For me there were some tests that failed with stage 2 in libstd for some reason... iirc, they had to do with networking and locking...
cc @rust-lang/wg-rustc-dev-guide @rust-lang/compiler @rust-lang/libs (not sure if I missed any other teams)
The eventual goal is to replace CONTRIBUTING.md and the README.md in rust-lang/rust with pointers to this document.
This is still very WIP, but it aims to address some of the concerns that WG-rustc-dev-guide was feeling with respect to organization, as well as some of the concerns that have come up in the contributor survey.
Feedback is welcome, especially on the structure and content of the document (e.g. if you feel something else should be added, or something should be removed)! It still pretty early, so I expect that this will change a bit before things settle.
Also, please excuse me if there is some delay in responding.