-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Check rustbook links on all platforms when running locally #62950
Conversation
cc @mark-i-m |
(rust_highfive has picked a reviewer for you, use r? to override) |
|
We have historically disabled this because of the significant amount of time it takes to execute on CI. Is there a good grasp on how much time this adds to CI builders to execute this step? |
It's hard to tell because it's not a separate step (it's part of rustbook). @ehuss had troubles with current approach:
I haven't thought about it earlier but maybe it could be disabled on CI (except 1 job) and still ran locally? My idea is to make linkcheck a feature of rustbook, it would be enabled unless some kind of environment variable or configuration option is present. |
That seems reasonable to me, but I think it'd also be reasonable to do a little work and basically execute this locally to see how long it takes to see whether it's necessary. |
On my box (
Linkcheck was nuked out of existence with this diff: https://gist.github.com/mati865/00fd7281723a4922a719902ad5c3d4ce So it will increase build time by ~1.6%. |
That looks like linkcheck is taking 300s (5 minutes), is that right? If so that's definitey much too long to execute on each builder running tests. |
I don't know about Rust CI but on my machine it increased build time by 1.6% (25,7 seconds) to 27 minutes 51 seconds total time. |
Sorry I'm having trouble parsing this. Is the data here showing that your build increased by 30 seconds after enabling this? If that's the case this seems fine to enable, that's just margin of error. |
Generally you want to look for Ryzen 2700X is much more powerful CPU than what Rust have as the CI so it will definitely take longer but I'm not able tell how much longer. |
So.... your local build took 30 seconds more, right? If that's true then my thinking is the same, 30 seconds of a 30 minute build is margin of error which means this has basically no real impact. I'm just trying to confirm because I literally didn't understand "27,51s" before, I haven't seen that syntax to denote "27 minutes and 51 seconds" before myself. |
Right, it should have been: "Increased build time by 1.6% (25,7 seconds) from 27 minutes 25 seconds to 27 minutes 51 seconds total time. |
@bors: r+ |
📌 Commit 2282c213e126362433f2b3a1a9434433b14fdb4a has been approved by |
Sorry, I'm a bit late to the game. The CI time for the guide is about 2.5m
right now. I actually would prefer not to run linkcheck on all platforms
because it increases the odds of getting a spurious failure or rate
limiting. I like the idea mentioned above of turning it off with an env var
for most builders.
…On Fri, Aug 2, 2019, 11:29 AM Alex Crichton ***@***.***> wrote:
@bors <https://github.com/bors>: r+
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#62950?email_source=notifications&email_token=ACDLHQFPCV42WVAEYBGNUU3QCROG3A5CNFSM4IGTKIW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3OHJOY#issuecomment-517764283>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACDLHQEM5LI6DWE66NB64BLQCROG3ANCNFSM4IGTKIWQ>
.
|
@bors: r- |
Should be ready for review. |
Changed the way the feature is enabled and improved CI detection tu reuse bootstrap check in the last commit. |
@bors: delegate+ Looks good to me! |
✌️ @mati865 can now approve this pull request |
Squashed and rebased. @bors r=alexcrichton |
📌 Commit c7e16c5 has been approved by |
Check rustbook links on all platforms when running locally cc rust-lang#62739
Rollup of 7 pull requests Successful merges: - #62672 (Deprecate `try!` macro) - #62950 (Check rustbook links on all platforms when running locally) - #63114 (Remove gensym in format_args) - #63397 (Add tests for some ICEs) - #63403 (Improve test output) - #63404 (enable flt2dec tests in Miri) - #63407 (reduce some test sizes in Miri) Failed merges: r? @ghost
cc @ehuss |
cc #62739