Skip to content
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

Distribute json doc #101799

Merged
merged 1 commit into from
Sep 19, 2022
Merged

Distribute json doc #101799

merged 1 commit into from
Sep 19, 2022

Conversation

LukeMathWalker
Copy link
Contributor

Overview

We add a new component, rust-json-docs, to distribute the JSON version of rustdoc's output for public compiler crates (i.e. std, alloc, proc_macro, core and test).
As discussed in #101383, we do not bundle this up as part of the existing rust-docs component since rustdoc's JSON format is still unstable.

Open questions / Doubts

I tried my best, but I never touched this codebase and I couldn't find much documentation on how dist works - I pattern-matched existing code, which might have led to some non-sensical choices in the eyes of people more familiar with the codebase. In particular, I am not sure if my choice of adding a new config flag is appropriate or if the decision to build/not build the JSON docs is more appropriately gated by one of the existing flags.
Any suggestion is more than welcome.

Closes #101383

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 14, 2022
@rust-highfive
Copy link
Collaborator

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 14, 2022
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

cc @Mark-Simulacrum

@aDotInTheVoid
Copy link
Member

@rustbot modify labels: +A-rustdoc-json

@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Sep 14, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks roughly right, but I think we can simplify it quite a bit.

Once you've made the change that builds the JSON docs whenever the normal docs are built, I can run @bors try to generate the rust-json-docs component and verify that it has the output you expect. You can also run x dist rust-json-docs locally and look at the tarball in build/dist.

src/bootstrap/dist.rs Outdated Show resolved Hide resolved
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
config.toml.example Outdated Show resolved Hide resolved
src/bootstrap/config.rs Outdated Show resolved Hide resolved
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2022
@jyn514
Copy link
Member

jyn514 commented Sep 14, 2022

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2022
@rust-log-analyzer

This comment has been minimized.

@LukeMathWalker
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 15, 2022
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks in a lot better shape, thanks :) just a few small comments.

src/bootstrap/doc.rs Outdated Show resolved Hide resolved
src/bootstrap/doc.rs Show resolved Hide resolved
Comment on lines +573 to +611
if requested_crates.iter().any(|p| p == krate) {
// No need to document more of the libraries if we have the one we want.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks extremely suspicious, but it was already here before so no need to fix it here.

Copy link
Contributor Author

@LukeMathWalker LukeMathWalker Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this felt broken to me as well - especially considering that we have a vector of request crates but we abort as soon as one of those is built (instead of making sure that all of them are built).

src/bootstrap/doc.rs Show resolved Hide resolved
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
src/bootstrap/doc.rs Show resolved Hide resolved
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Sep 15, 2022

r=me with that last comment addressed and the commits squashed :) thanks for sticking with this!

@LukeMathWalker
Copy link
Contributor Author

r? @jyn514

@LukeMathWalker
Copy link
Contributor Author

Thank you for guiding me through it @jyn514!

@jyn514
Copy link
Member

jyn514 commented Sep 16, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 16, 2022

📌 Commit ccd4383 has been approved by jyn514

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2022
@LukeMathWalker
Copy link
Contributor Author

I've added a check before attempting the deletion (alternatively I can add try_remove_dir method and handle the error that is returned).
@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 17, 2022
@jyn514
Copy link
Member

jyn514 commented Sep 17, 2022

I noticed that after this change we'll print documenting std twice. Can you fix that while you're at it?

…ed documentation for std crates in nightly toolchains.

We also add a new flag to `x doc`, `--json`, to render the JSON-formatted version alongside the HTML-formatted one.
@LukeMathWalker
Copy link
Contributor Author

Fixed it! @jyn514

@jyn514
Copy link
Member

jyn514 commented Sep 17, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2022

📌 Commit 235dcce has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2022
@bors
Copy link
Contributor

bors commented Sep 18, 2022

⌛ Testing commit 235dcce with merge 8b5ebc275c2ef02347a6a6c1496ea49b411259bd...

@bors
Copy link
Contributor

bors commented Sep 18, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 18, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@jyn514
Copy link
Member

jyn514 commented Sep 18, 2022

curl: (56) OpenSSL SSL_read: Connection reset by peer, errno 54

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2022
@bors
Copy link
Contributor

bors commented Sep 19, 2022

⌛ Testing commit 235dcce with merge c8e12cc...

@bors
Copy link
Contributor

bors commented Sep 19, 2022

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing c8e12cc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2022
@bors bors merged commit c8e12cc into rust-lang:master Sep 19, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 19, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c8e12cc): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This 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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.6% [1.9%, 3.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.7% [-2.8%, -2.5%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distribute documentation in JSON format via rustup for toolchain components
10 participants