-
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
rustbuild: Skip generating docs if the config disables them. #32718
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Thanks for the PR! I think the strategy here, however, may be the more manageable approach where we just don't generate these targets in those situations (but they can still be explicitly run if desired). Want to update the PR here to that strategy? May be able to land the two in parallel to see which hits master first. |
6487ed2
to
37b663e
Compare
Yeah, that does look a better approach. I've updated the branch to use that approach. I see that the other PR may be close to ready though, so if that's the case we can just close the PR and let the fix get submitted through the other one. |
Also note that it wasn't really clear to me if or how the build.config.compiler_docs variable should be used now. I'm not sure what steps constitute "compiler docs" vs non-compiler docs. |
@@ -148,7 +148,9 @@ fn top_level(build: &Build) -> Vec<Step> { | |||
src: Source::Llvm { _dummy: () }, | |||
target: &build.config.build, | |||
}; | |||
targets.push(t.doc(stage)); | |||
if build.config.docs { | |||
targets.push(t.doc(stage)); |
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 use 4 space tabs here as well?
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.
Done in latest commit. I'm assuming the commits will be squashed before being merged, right?
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.
Indeed!
☔ The latest upstream changes (presumably #32751) made this pull request unmergeable. Please resolve the merge conflicts. |
2a58367
to
133cabc
Compare
Could you squash the commits down into one as well? Also feel free to ping a PR after it's updated because unfortunately github doesn't send out notifications for that :( |
It looks like before these config variables weren't actually taken into account. This patch should make the build system skip over the documentation steps correctly.
133cabc
to
2805e83
Compare
Squashed into a single commit now. |
rustbuild: Skip generating docs if the config disables them. r? @alexcrichton
r? @alexcrichton