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

Only check the compiler and standard library before documenting them (take 2) #95450

Merged
merged 1 commit into from
Apr 16, 2022
Merged

Only check the compiler and standard library before documenting them (take 2) #95450

merged 1 commit into from
Apr 16, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 29, 2022

Trying #88675 again.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2022
@jyn514
Copy link
Member Author

jyn514 commented Mar 29, 2022

r? @ghost

@jyn514 jyn514 marked this pull request as draft March 29, 2022 21:55
@jyn514 jyn514 changed the title Only check the compiler and standard library before documenting them (take 2) [wip] Only check the compiler and standard library before documenting them (take 2) Mar 29, 2022
@compiler-errors
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Mar 29, 2022

⌛ Trying commit 69e8a815f045efca712f5149853dd73e34b68967 with merge 23522fca09aeb8a44837a2b203076ee6a491b431...

@bors
Copy link
Contributor

bors commented Mar 29, 2022

☀️ Try build successful - checks-actions
Build commit: 23522fca09aeb8a44837a2b203076ee6a491b431 (23522fca09aeb8a44837a2b203076ee6a491b431)

@jyn514
Copy link
Member Author

jyn514 commented Mar 30, 2022

I have no idea how to test this :( the error in #88675 only showed up in the full bors test, and I've been unable to reproduce it locally. I tried docker but I can't run arm64 containers locally: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/How.20to.20reproduce.20try.20run.20error/near/277207099

I guess you could just approve the PR and hope it works? At least it would let me download the build log, the old one's expired.

@Mark-Simulacrum
Copy link
Member

I'm happy to approve it to try it on CI but you'll need to revert the CI changes so we don't by accident land those :)

@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 Apr 10, 2022
@jyn514
Copy link
Member Author

jyn514 commented Apr 10, 2022

I'm happy to approve it to try it on CI but you'll need to revert the CI changes so we don't by accident land those :)

Sure, done :) and if you have suggestions for how to test this without a full bors run, that would be extremely helpful too.

@jyn514
Copy link
Member Author

jyn514 commented Apr 10, 2022

@rustbot ready

@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 Apr 10, 2022
@Mark-Simulacrum
Copy link
Member

re:full CI, try builds should be equivalent, unlike on-PR builds which have a different and more limited set of permissions.

The changes here seem broadly correct though so I'm happy to run full CI and we can move to try builds for faster iteration after that (I can delegate or whatever the PR if you don't have permissions). @bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2022

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove [WIP] from this PR's title when it is ready for review.

@Mark-Simulacrum
Copy link
Member

@bors delegate+

@bors
Copy link
Contributor

bors commented Apr 11, 2022

✌️ @jyn514 can now approve this pull request

@jyn514
Copy link
Member Author

jyn514 commented Apr 11, 2022

@rustbot label -S-waiting-on-review +S-waiting-on-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 Apr 11, 2022
@jyn514 jyn514 changed the title [wip] Only check the compiler and standard library before documenting them (take 2) Only check the compiler and standard library before documenting them (take 2) Apr 12, 2022
@jyn514
Copy link
Member Author

jyn514 commented Apr 12, 2022

@bors r=Mark-Simulacrum rollup=never

@bors
Copy link
Contributor

bors commented Apr 12, 2022

📌 Commit bbdcd25a27e2b53d8f740dfe470a51cee60669ca has been approved by Mark-Simulacrum

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 12, 2022
@bors
Copy link
Contributor

bors commented Apr 13, 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 Apr 13, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member Author

jyn514 commented Apr 13, 2022

Ok great this is the error from last year - I'll save the log file this time and see if I can figure out how to reproduce.

This was referenced Apr 13, 2022
Open
Closed
@jyn514
Copy link
Member Author

jyn514 commented Apr 15, 2022

Wasn't able to reproduce, but did come up with the idea this might be because both the compile::Rustc and check::Rustc artifacts are in the sysroot. Changed tool_doc! to do a full compile instead of a check to see if it avoids the issue. doc compiler will still only check it.

@jyn514
Don't build the library and standard library before documenting them
dd0ad73 
Rustdoc doesn't require the build artifacts to generate the docs, and
especially in the case of rustc, it greatly increases the time needed to
run the build.

- Statically ensure that only the top_stage of a tool is documented

If another part of rustbuild tried to document a different stage, it
would run into errors because `check::Rustc` unconditionally uses the
top stage.

- Try building rustc instead of checking to avoid duplicate artifacts

Tries to workaround the following error:
```
error[E0464]: multiple matching crates for `rustc_ast`
  --> src/librustdoc/lib.rs:40:1
   |
40 | extern crate rustc_ast;
   | ^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: candidates:
           crate `rustc_ast`: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_ast-6d7c193782263d89.rlib
           crate `rustc_ast`: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_ast-e5d09eda5beb759c.rmeta
```
@jyn514
Copy link
Member Author

jyn514 commented Apr 15, 2022

@bors r=Mark-Simulacrum rollup=never

If this doesn't work, I think I'm going to try and merge the libstd changes separately, they should be less likely to fail.

@bors
Copy link
Contributor

bors commented Apr 15, 2022

📌 Commit dd0ad73 has been approved by Mark-Simulacrum

@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 Apr 15, 2022
@bors
Copy link
Contributor

bors commented Apr 16, 2022

⌛ Testing commit dd0ad73 with merge 27490eb...

@bors
Copy link
Contributor

bors commented Apr 16, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 27490eb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 16, 2022
@bors bors merged commit 27490eb into rust-lang:master Apr 16, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 16, 2022
@jyn514 jyn514 deleted the faster-doc branch April 16, 2022 04:32
@jyn514 jyn514 mentioned this pull request Apr 16, 2022
Closed
@bors bors mentioned this pull request Apr 16, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (27490eb): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 1 0 2 1
mean2 0.9% 0.4% N/A -1.2% 0.9%
max 0.9% 0.4% N/A -1.2% 0.9%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added the perf-regression Performance regression. label Apr 16, 2022
@jyn514
Copy link
Member Author

jyn514 commented Apr 16, 2022

I think these are just noise. The bootstrap changes don't affect build at all, only documenting.

@jyn514 jyn514 mentioned this pull request Apr 19, 2022
Closed
@pnkfelix
Copy link
Member

Visiting for weekly performance triage.

  • hypothesized to be noise and pnkfelix doesn't have time to dig more deeply.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 21, 2022
@rylev rylev mentioned this pull request Apr 25, 2022
Merged
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
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
10 participants
@jyn514 @rust-highfive @compiler-errors @bors @Mark-Simulacrum @rust-log-analyzer @ChrisDenton @rust-timer @pnkfelix @rustbot