Skip to content

BuildClient: Add simple retry policy for failed doc builds #544

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

Merged
merged 2 commits into from
Oct 23, 2016

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Sep 15, 2016

I've been noticing a fair number of Hackage packages lacking
documentation due to apparently transient issues (e.g. somehow a package
entering the build queue despite having no entry in the package index,
see #543).

Here we add a simple retry policy; attempting to build a failing package
up to a given number of attempts. The number of failures is now
persisted in the failure file.

@bgamari
Copy link
Contributor Author

bgamari commented Sep 15, 2016

Currently I lack a plan for testing this.

I've been noticing a fair number of Hackage packages lacking
documentation due to apparently transient issues (e.g. somehow a package
entering the build queue despite having no entry in the package index,
see haskell#543).

Here we add a simple retry policy; attempting to build a failing package
up to a given number of attempts. The number of failures is now
persisted in the failure file.
@bgamari bgamari force-pushed the doc-builder-retry branch 3 times, most recently from bdd72fa to a8c1073 Compare October 12, 2016 17:59
Due to caching sometimes the package repository state may lag behind the
documentation index. Consequently, we make sure that the packages we are
going to build actually appear in the repository before building. See
Issue haskell#543.
@bgamari bgamari changed the title [WIP] BuildClient: Add simple retry policy for failed doc builds BuildClient: Add simple retry policy for failed doc builds Oct 23, 2016
@@ -869,7 +892,8 @@ validateOpts args = do
bo_dryRun = flagDryRun flags,
bo_prune = flagPrune flags,
bo_username = flagUsername flags,
bo_password = flagPassword flags
bo_password = flagPassword flags,
bo_buildAttempts = fromMaybe 10 $ flagBuildAttempts flags
Copy link
Contributor

Choose a reason for hiding this comment

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

seems a little high as a default, no?

parseRepositoryIndices = do
cabalDir <- getAppUserDataDirectory "cabal/packages"
cacheDirs <- listDirectory cabalDir
indexFiles <- filterM doesFileExist $ map (\dir -> cabalDir </> dir </> "00-index.cache") cacheDirs
Copy link
Contributor

Choose a reason for hiding this comment

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

This format is actually private to cabal, and does change (it's already changed in cabal-install-1.25). On the other hand the 00-index.tar or the 01-index.tar is a stable format. You can use the code from the mirror client (in the same repo) for this.

Copy link
Member

@hvr hvr Oct 23, 2016

Choose a reason for hiding this comment

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

otoh, the on-disk format for 00-index.cache is supposed to remain backward compatible, otherwise different cabal clients would step on each other's toe ... For 01-index.cache it's a different story though :-)

Copy link
Member

@hvr hvr left a comment

Choose a reason for hiding this comment

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

lgtm in general


-- | Parse the @00-index.cache@ file of the available package repositories.
parseRepositoryIndices :: IO (S.Set PackageIdentifier)
parseRepositoryIndices = do
Copy link
Member

Choose a reason for hiding this comment

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

Note that this will likely break for secure repos as the filename and on-disk format for 01-index.* changed

In future cabal versions we should extend or add a cabal list variant that dumps the required information (including additional meta-data such as revision number and/or last-revision-timestamp, and maybe also repository provenance) so that we don't need to understand the .cache format

@dcoutts
Copy link
Contributor

dcoutts commented Oct 23, 2016

Modulo comments above, the change to add retries and to make sure the package is already in the index is a good idea.

@bgamari bgamari merged commit 3754bdf into haskell:master Oct 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants