-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Host compiler documentation #49193
Host compiler documentation #49193
Conversation
I'm expecting that this will have some issues (particularly in the naming of some of the steps) and I didn't have an opportunity to throughly test the |
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! Have you tried running this locally as well to ensure the tarball is generated correctly?
@@ -84,7 +84,8 @@ ENV HOSTS=x86_64-unknown-linux-gnu | |||
ENV RUST_CONFIGURE_ARGS \ | |||
--enable-full-tools \ | |||
--enable-sanitizers \ | |||
--enable-profiler | |||
--enable-profiler \ | |||
--enable-compiler-args |
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.
s/args/docs/
I think?
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.
Oops, I think you're right, will change 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.
Fixed.
@@ -620,19 +625,86 @@ impl Step for Rustc { | |||
let mut cargo = builder.cargo(compiler, Mode::Librustc, target, "doc"); | |||
compile::rustc_cargo(build, &mut cargo); | |||
|
|||
if build.config.compiler_docs { |
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.
Could you expand a bit on how come this step was separated? I'd naively expect it to be ok if it continued to just be one step that internally dispatched to all crates or just proc_macro
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.
When the standard documentation is being generated - this is done by merging the standard library, test and procedural macro documentation (normally, the compiler_docs
flag would add everything else here too). This was achieved through symlinking docs
to a shared folder for those three steps.
In order to produce the compiler documentation such that we would still have standard documentation separately which contains the same crates as it did pre-PR, I had to ensure that the symlink directory was different when generating all the compiler documentation. This is because the distribution step would a copy from that generated documentation and that would contain the compiler documentation too if it wasn't generated into somewhere else (this is because all documentation steps happen before dist steps).
(It occurs to me now when writing this that since each doc step copies from the shared symlink directory to its output directory at the end of each step that we could still use the same shared folder which might speed things up. This is because the dist step for standard documentation will use the folder that was copied before compiler documentation was added - will try this). Done this now.
I assumed that we'd want the standard documentation to still include the proc_macro
crate and so I kept one step that would generate that into the shared location and created another that would generate the compiler documentation into the separate location. These two locations then produce the two tarballs in the dist step. These could be one step that just called to cargo doc
twice.
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.
Updated that commit to contain the improvement I noticed in the above comment.
src/bootstrap/doc.rs
Outdated
// Like with libstd above if compiler docs aren't enabled then we're not | ||
// documenting internal dependencies, so we have a whitelist. | ||
cargo.arg("--no-deps"); | ||
for krate in &["proc_macro"] { |
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.
... why? 😕
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.
Currently, the documentation on doc.rust-lang.org
contains the standard library, test and proc_macro
. This step generates the proc_macro
documentation into the same folder that already contains the standard library and test documentation. At the end of this step, this documentation is copied into the doc
folder and joined by the various books/error index/etc. - this is what produces the tarball and the contents match the contents before this PR.
After the above, we add the compiler crates in another step to the original folder (not the doc
folder it was copied into) and then make a new compiler-doc
folder from that. This produces the compiler docs tarball.
In order to actually perform the above, I modified the existing step that produced compiler documentation so that it would only include the docs (ie. only proc_macro
for the original tarball; and added a new step that produced the rest.
Does this clarify why?
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.
@davidtwco Sorry, let me clarify.
Why iterate on an array with a single element when you could just write cargo.arg("-p").arg("proc_macro")
directly? The step name doesn't suggest you'll need to extend this array.
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.
Before this PR, when compiler docs were disabled, a whitelist of crates from the compiler were documented for the standard documentation. This was clear from the context and that it was an array. However, as you point out, that context has been lost somewhat.
I had mentioned in my initial comment that I wasn't sure about the name of this step. I named it ProcMacro
just so that it had a name for me to work with, in practice it should be PublicRustc
or WhitelistRustc
or something like that since it could contain more crates that we choose to include in the standard available documentation.
I'm not sure what name is best for this step but I'll change it and clarify the point of it in a comment.
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.
Modified that commit to clarify this.
Ok thanks for the explanations @davidtwco! This looks good to me with all that in mind, so let's merge! Before that though let's give this a spin to make sure it'll work @bors: try |
Host compiler documentation Fixes #29893. Rust Central Station PR: rust-lang/rust-central-station#40 r? @alexcrichton
💔 Test failed - status-travis |
@alexcrichton I'm not sure that the error on Travis is related to the contents of this PR? |
It looks like we're uploading a bit too much? The raw HTML files look to be getting uploaded instead of just the tarballs |
@alexcrichton Ah, I saw the following and assumed that was the issue:
Locally, I was seeing the folder structure with various tar archives for the documentation and compiler, etc. But I was also seeing a folder with the raw documentation for both Lines 123 to 129 in 1e73c1d
The above is from the existing So I assume that it is intended that we produce regular directories alongside the tarballs? Perhaps that will need to change if the Also, I've noticed another issue locally - it seems that while the source directories for the archives have the files I expected in them, the actual archives somehow combine the files. I hadn't checked the archives before (my bad), just looked at the source directories all the way through the dist process and assumed the |
Hm yeah I'm not really sure what's up? In any case that's a very outdated comment this point, we only produce/upload tarballs from the builders nowadays |
I've pushed modified commits that remove that section since it doesn't seem like we need it. I think I've also fixed the other issue I'd noticed. |
@bors: try |
Host compiler documentation Fixes #29893. Rust Central Station PR: rust-lang/rust-central-station#40 r? @alexcrichton
💔 Test failed - status-travis |
@bors retry |
Host compiler documentation Fixes #29893. Rust Central Station PR: rust-lang/rust-central-station#40 r? @alexcrichton
☀️ Test successful - status-travis |
After chatting with @alexcrichton on Gitter, we've decided to remove the std and test documentation from the compiler tarball. Pushed a commit that does this. |
@bors: try |
⌛ Trying commit 73fa6d5 with merge fd9b5ca0fa489432dfb0fa200555316a5ef04a03... |
☀️ Test successful - status-travis |
@bors: try |
er, wrong pr... |
@bors: r+ Ok looks great! |
📌 Commit 73fa6d5 has been approved by |
Host compiler documentation Fixes rust-lang#29893. Rust Central Station PR: rust-lang/rust-central-station#40 r? @alexcrichton
Host compiler documentation Fixes rust-lang#29893. Rust Central Station PR: rust-lang/rust-central-station#40 r? @alexcrichton
Host compiler documentation Fixes rust-lang#29893. Rust Central Station PR: rust-lang/rust-central-station#40 r? @alexcrichton
Host compiler documentation Fixes rust-lang#29893. Rust Central Station PR: rust-lang/rust-central-station#40 r? @alexcrichton
Fixes #29893. Rust Central Station PR: rust-lang/rust-central-station#40
r? @alexcrichton