Rework new-build failure reporting to include build logs #3686
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I think these names are now a bit more induitive / less confusing and a bit more consistent. We now have BuildOutcome(s) to mean either failure or a build result, and BuildResult to mean the result of a non-failing build.
We're going to alter and extend the BuildResult and BuildFailure types for the new-build code, so we cannot share those types with the old install code. This patch doesn't change the structure, just redefines them locally and switches uses to record style so we can add new fields easily.
In both cases it's optional and only added when the log file is used. This will be used later in reporting the build outcomes.
The PlanningFailed case does not happen. The ReplFailed, HaddocksFailed cases were previously covered by BuildFailed but they're worth disinguishing.
Reporting build logs is important as we otherwise have no real info for why deps failed to build. It does make the presentation more difficult however because build logs can be long and in principle there can be several, which means it can take up more than a single screen in a console. The first thing users notice is typically the last few messages, so in the case that we're presenting long build logs then its important to also include a short summary at the end. So our approach is this: for packages where we want to show a build log, we dump those first, in full, each with a header to indicate which package it is, log file name (for later reference), plus any extra detail we have from the phase of the failure or the exception. Then after all build logs we end with a short summary of the failure(s). For packages where we do not show a build log (e.g. local packages that dump live to the console) we only present a summary at the end, but we include a little more detail than for the packages that had a build log since this is the only thing we report for them. So we include details of the exception.
|
Compare the output from #3394: with the output from a similar test case now (provoking a failure in zip-archive-0.3.0.2) |
|
Is it sufficiently clear from the summary at the end that there are further details above? Should we explicitly point to it? e.g. |
+1, I think this is a good idea. |
| @@ -612,7 +663,8 @@ rebuildTargets verbosity | |||
| installPlan pkgsBuildStatus $ \downloadMap -> | |||
|
|
|||
| -- For each package in the plan, in dependency order, but in parallel... | |||
| InstallPlan.execute jobControl keepGoing (DependentFailed . packageId) | |||
| InstallPlan.execute jobControl keepGoing | |||
| (BuildFailure Nothing . DependentFailed . packageId) | |||
There was a problem hiding this comment.
Probably is more forward looking to have a mkBuildFailureNoLog function instead.
|
OK, looks good to me. |
|
@dcoutts If you restart the AppVeyor build it should work now. |
Make annotateFailure take a maybe log file, and add a annotateFailureNoLog for the other case. This is a little more future proof and simplifies things at the call sites.
|
LGTM as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the follow-up to #3665 and should address all the issues raised there.
Reporting build logs is important as we otherwise have no real info for why deps failed to build. It does make the presentation more difficult however because build logs can be long and in principle there can be several, which means it can take up more than a single screen in a console. The first thing users notice is typically the last few messages, so in the case that we're presenting long build logs then its important to also include a short summary at the end.
So our approach is this: for packages where we want to show a build log, we dump those first, in full, each with a header to indicate which package it is, log file name (for later reference), plus any extra detail we have from the phase of the failure or the exception. Then after all build logs we end with a short summary of the failure(s). For packages where we do not show a build log (e.g. local packages that dump live to the console) we only present a summary at the end, but we include a little more detail than for the packages that had a build log since this is the only thing we report for them. So we include details of the exception.
The PR starts with a few patches to rename things and rearrange the plumbing to get the info to the right place.