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.

Sign up for GitHub

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

Jump to bottom

copy doc output files by format #104286

Merged
merged 2 commits into from
Nov 23, 2022
Merged

copy doc output files by format #104286

merged 2 commits into from
Nov 23, 2022

Conversation

onur-ozkan
Copy link
Member

This pr provides copying doc outputs by checking output format without removing output directory on each trigger.

Resolves #103785

@rustbot
Copy link
Collaborator

rustbot commented Nov 11, 2022

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 11, 2022
onur-ozkan
onur-ozkan commented Nov 11, 2022
View reviewed changes
src/bootstrap/doc.rs Outdated
Comment on lines 587 to 590
DocumentationFormat::HTML if extension != Some("json") => {
builder.copy(&path, &dst)
}
DocumentationFormat::JSON
if extension == Some("json") || name.to_str() == Some(".stamp") =>
{
builder.copy(&path, &dst)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if this conditions will be enough for the purpose. What do you think ? @jyn514

@onur-ozkan onur-ozkan changed the title copy doc output files by format WIP: copy doc output files by format Nov 11, 2022
@onur-ozkan onur-ozkan force-pushed the fix-doc-bootstrap-recompilation branch from c07f8e9 to 89d4550 Compare November 13, 2022 17:44
@onur-ozkan onur-ozkan changed the title WIP: copy doc output files by format copy doc output files by format Nov 13, 2022
@onur-ozkan
Copy link
Member Author

pr is ready for review @Mark-Simulacrum @jyn514

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@onur-ozkan
copy doc output files by format r=ozkanonur
0a275ab 
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan force-pushed the fix-doc-bootstrap-recompilation branch from c6d6b4b to 0a275ab Compare November 13, 2022 18:32
@Mark-Simulacrum
Copy link
Member

Is there a reason we need to have a shared output directory for our invocations of cargo doc for JSON and not JSON? That seems much less picky in terms of picking file types out by extension...

In particular, it seems like the HTML format has some JSON files so the approach of this PR won't quite work:

~/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/share$ find doc/rust/html | grep json
doc/rust/html/book/searchindex.json
doc/rust/html/book/second-edition/searchindex.json
doc/rust/html/book/2018-edition/searchindex.json
doc/rust/html/book/first-edition/searchindex.json
doc/rust/html/rust-by-example/searchindex.json
doc/rust/html/error_codes/searchindex.json
doc/rust/html/cargo/searchindex.json
doc/rust/html/clippy/searchindex.json
doc/rust/html/rustc/searchindex.json
doc/rust/html/rustc/json.html
doc/rust/html/embedded-book/searchindex.json
doc/rust/html/rustdoc/searchindex.json
doc/rust/html/nomicon/searchindex.json
doc/rust/html/src/test/formatters/json.rs.html
doc/rust/html/unstable-book/searchindex.json
doc/rust/html/reference/searchindex.json
doc/rust/html/edition-guide/searchindex.json

@Mark-Simulacrum Mark-Simulacrum 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 Nov 13, 2022
@jyn514
Copy link
Member

jyn514 commented Nov 14, 2022

Hmm, won't it hurt caching if cargo has to rebuild the dependencies in between? We can only control the target directory cargo uses, not the --out-dir it passes to rustdoc.

I agree this seems to cause issues though ... maybe a stamp file is better after all.

@onur-ozkan
Copy link
Member Author

In particular, it seems like the HTML format has some JSON files so the approach of this PR won't quite work:

~/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/share$ find doc/rust/html | grep json
doc/rust/html/book/searchindex.json
doc/rust/html/book/second-edition/searchindex.json
doc/rust/html/book/2018-edition/searchindex.json
doc/rust/html/book/first-edition/searchindex.json
doc/rust/html/rust-by-example/searchindex.json
doc/rust/html/error_codes/searchindex.json
doc/rust/html/cargo/searchindex.json
doc/rust/html/clippy/searchindex.json
doc/rust/html/rustc/searchindex.json
doc/rust/html/rustc/json.html
doc/rust/html/embedded-book/searchindex.json
doc/rust/html/rustdoc/searchindex.json
doc/rust/html/nomicon/searchindex.json
doc/rust/html/src/test/formatters/json.rs.html
doc/rust/html/unstable-book/searchindex.json
doc/rust/html/reference/searchindex.json
doc/rust/html/edition-guide/searchindex.json

May I know how did you achieve this?

@Mark-Simulacrum
Copy link
Member

The files in question can be installed with rustup +stable component add rust-docs and then the command and where I ran it from is in the snippet.

@Mark-Simulacrum
Copy link
Member

Hmm, won't it hurt caching if cargo has to rebuild the dependencies in between?

I think in practice re-building the standard library and its dependencies shouldn't take long enough to be worried about it for now -- and if we follow the suggestion around different destinations via symlink updates or similar (below) then it might not need to rebuild dependencies.

We can only control the target directory cargo uses, not the --out-dir it passes to rustdoc.

I think we do some symlink'ing and such to adjust where the out-dir passed to rustdoc actually ends up, fwiw, but it's not exactly the most ideal thing.

@onur-ozkan
Copy link
Member Author

I think in practice re-building the standard library and its dependencies shouldn't take long enough to be worried about it for now -- and if we follow the suggestion around different destinations via symlink updates or similar (below) then it might not need to rebuild dependencies.

Should I work on fixing the issue you mentioned then?

@jyn514
Copy link
Member

jyn514 commented Nov 17, 2022

@ozkanonur yes please, if you use separate --target-dir flags for JSON and non-json output that should sidestep this issue altogether (and we can worry about the compile time later, I think Mark is right it won't be a big deal in practice).

@onur-ozkan
refactor doc copying process
7e28df9 
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

We can only control the target directory cargo uses, not the --out-dir it passes to rustdoc.

I tried to check if I can play with --out-dir by checking doc format, however I couldn't figure how -o is passed to here

rust/src/librustdoc/lib.rs

Line 705 in e07425d

fn main_args(at_args: &[String]) -> MainResult {

At least now I have more experience about the rustdoc process pipelines.

if you use separate --target-dir flags for JSON and non-json output that should sidestep this issue altogether

With the last commit I aplied it for std doc(I think we don't need this for compiler-docs since it's always including html docs. Please correct me if I am wrong.).

@jyn514
Copy link
Member

jyn514 commented Nov 23, 2022

This looks great! thank you :)

@bors r+

@bors
Copy link
Contributor

bors commented Nov 23, 2022

📌 Commit 7e28df9 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 23, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Nov 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 23, 2022
@bors
Auto merge of rust-lang#104776 - Dylan-DPC:rollup-rf4c2u0, r=Dylan-DPC
d121aa3 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#104269 (Fix hang in where-clause suggestion with `predicate_can_apply`)
 - rust-lang#104286 (copy doc output files by format)
 - rust-lang#104509 (Use obligation ctxt instead of dyn TraitEngine)
 - rust-lang#104721 (Remove more `ref` patterns from the compiler)
 - rust-lang#104744 (rustdoc: give struct fields CSS `display: block`)
 - rust-lang#104751 (Fix an ICE parsing a malformed attribute.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d3e9191 into rust-lang:master Nov 23, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
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
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.

x doc std unconditionally recompiles the standard library each time
6 participants
@onur-ozkan @rustbot @rust-log-analyzer @Mark-Simulacrum @jyn514 @bors