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

Don't spend an hour on crates that timeout during the build #1910

Open
Nemo157 opened this issue Nov 15, 2022 · 5 comments · May be fixed by #2185
Open

Don't spend an hour on crates that timeout during the build #1910

Nemo157 opened this issue Nov 15, 2022 · 5 comments · May be fixed by #2185
Labels
A-builds Area: Building the documentation for a crate

Comments

@Nemo157
Copy link
Member

Nemo157 commented Nov 15, 2022

Currently if we have a build that hits the 15 minute timeout (e.g. app-db-schema@0.1.2) we will end up spending an hour total attempting to build the release. We do 4 builds total, each with the full timeout available to them:

  1. With the crates Cargo.lock:
    a. Generate coverage data
    b. Build docs
  2. After deleting the lock:
    a. Generate coverage data
    b. Build docs

It would be better to only generate the coverage data if the build succeeded, so we would only attempt builds 1.b. and 2.b. before deciding it timed out, but that has issues

docs.rs/src/docbuilder/rustwide_builder.rs

Lines 658 to 660 in 2e5ef9b

// we have to run coverage before the doc-build because currently it
// deletes the doc-target folder.
// https://github.com/rust-lang/cargo/issues/9447

One idea would be to skip the subsequent steps if one fails because of a timeout rather than a build error. It seems unlikely that unlocking the crate will turn a timeout into a successful build, or that rustdoc --show-coverage would somehow be the cause of a timeout rather than it being one of the dependencies.
@syphar syphar added the A-builds Area: Building the documentation for a crate label Nov 16, 2022
@syphar
Copy link
Member

syphar commented Nov 16, 2022

I remember that the whole build-attempt topic predates most (all?) of us.

To ask the "real" question:
what kind of errors are actually transient, so would be solved by just trying again?

I imagine network errors, but these won't happen in the docker container itself, but only outside of in in our builder.

So what remains?

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 16, 2022

This isn't actually related to retrying the build attempts, these are 4 invocations of cargo rustdoc from within the same build-attempt.

In this case it is actually possible that the 4th will succeed in very niche cases; the first two builds might timeout because of a locked dependency that times out, the third build might hit a bug in rustdoc specific to --show-coverage that causes it to timeout, then the 4th build finally succeeds and gives us docs. I think that is unlikely enough that we shouldn't spend the extra build time on supporting it.

@syphar
Copy link
Member

syphar commented Nov 16, 2022

I see there is still very much I don't know about the build process :)

So in extreme cases we would 4 times 4 tries?

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 16, 2022

No, in the end it 'successfully fails' so we don't increment the attempt counter and retry.

@syphar
Copy link
Member

syphar commented Nov 16, 2022

Ah, you're right.

@Nemo157 Nemo157 linked a pull request Aug 7, 2023 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-builds Area: Building the documentation for a crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change timeout to apply to the overall build process, not per-build Nemo157/docs.rs
2 participants
@Nemo157 @syphar