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.

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

Make the build output the WiX compilation log if it failed. #4831

Merged
merged 2 commits into from
Sep 14, 2017

Conversation

bergmeister
Copy link
Contributor

Although PR #4795 fixed the main issue of #4760 (to make the build red if there is a WiX compilation error), it did not fix the problem that currently the WiX log is not outputted at all and therefore one cannot see the compilation error in the log. This PR fixes this and outputs the WiX log only if no MSI could be produced because the WiX log is quite verbose (around 250 lines) and would be unnecessary noise in a green build.

The reason why the log is currently not displayed in the log is because the resulting object is piped directly to Write-Verbose but is of type array and therefore needs to be converted to a string using Out-String beforehand.

Before it was not outputting it not at all because the build output is an array that was piped directly to 'Write-Verbose' instead of converting it to a string using 'Out-String'
Because the WiX log is usually quite verbose (around 250 lines), the log is only shown if there must have been a compilation error due to the missing MSI
@msftclas
Copy link

@bergmeister,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

build.psm1 Outdated
@@ -1844,6 +1844,9 @@ function New-MSIPackage
}
else
{
$WiXHeatLog | Out-String | Write-Verbose
$WiXCandleLog | Out-String | Write-Verbose
$WiXLightLog | Out-String | Write-Verbose
Copy link
Member

Choose a reason for hiding this comment

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

I think you probably need Write-Verbose -Verbose, otherwise, the verbose message won't be written out unless New-MSIPackage is called with -Verbose.

Copy link
Contributor Author

@bergmeister bergmeister Sep 13, 2017

Choose a reason for hiding this comment

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

Ok. I fixed it. But why is the build script no being called using the -Verbose option from the top level? As long as advanced functions are used, the $VerbosePreference would be propagated through.

Copy link
Member

Choose a reason for hiding this comment

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

-Verbose is not specified when calling Invoke-AppveyorFinish and also not specified when calling Start-PSPackage, as well as when calling New-MSIPackage. Maybe it should be specified at the very top call.

But for this change, we'd better add -Verbose anyway because even if -Verbose is not specified when calling New-MSIPackage, it's still desired to write out the logs when no MSI was produced.

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

LGTM

@TravisEz13
Copy link
Member

TravisEz13 commented Sep 14, 2017

Our logs are already very long. If we are on AppVeyor and not local, it may be better to add these logs as artifacts and reference them, If local we can just reference the logs.
Here are the docs on how to push an artifact
https://www.appveyor.com/docs/build-worker-api/#push-artifact
You can test if we are in AppVeyor using an environment variable:
https://www.appveyor.com/docs/environment-variables/

I filed an issue for this:
#4838
Feel free to merge this.

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

It's an improvement. But these should be made into artifacts.

@bergmeister
Copy link
Contributor Author

@TravisEz13 : That's why the WiX log (around 250 lines) gets only output in the case of a failure to not pollute normal green builds.

@TravisEz13
Copy link
Member

unless the logs are very short, 1-5 line, an artifact would be better. The logs are very long and become difficult to find details in.

@adityapatwardhan adityapatwardhan merged commit d804d7f into PowerShell:master Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants