-
Notifications
You must be signed in to change notification settings - Fork 209
Fix queue failure, refactor it and add a bunch of tests #863
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
Conversation
@Nemo157 addressed or responded to all review comments. |
Moving the code to a separate struct allows to test it without setting up the whole DocBuilder stuff.
Co-authored-by: Nemo157 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to also run a local build to make sure things still work.
Oh wait, I can't run a local build because that doesn't go through the queue 🙃 hopefully your tests exercise everything. Can you also rebase over master so the Docker failure will go away? |
|
The only way to enable the queue builder is by running the daemon, which tries to build all 300,000 crates off of crates.io. Just downloading that number of crates takes several hours and it's a real waste of bandwidth. It would be great to refactor the builder and the daemon to be separate at some point. |
Addressed the feedback and moved the code from |
This PR fixes the documentation builder failing to get the next crate out of the queue, refactors the whole queue handling out of
DocBuilder
to allow easier tests, unifies all the queue handling into the newBuildQueue
and puts the number of attempts in the configuration. Along with the changes, tests were added for everything.This PR is best reviewed commit-by-commit.
r? @jyn514