Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
adityapatwardhan merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
bergmeister:LogWixErrorbergmeister/PowerShell:LogWixErrorCopy head branch name to clipboard
Sep 14, 2017
Merged

Make the build output the WiX compilation log if it failed.#4831
adityapatwardhan merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
bergmeister:LogWixErrorbergmeister/PowerShell:LogWixErrorCopy head branch name to clipboard

Conversation

@bergmeister

Copy link
Copy Markdown
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
Copy Markdown

@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

Comment thread build.psm1 Outdated
{
$WiXHeatLog | Out-String | Write-Verbose
$WiXCandleLog | Out-String | Write-Verbose
$WiXLightLog | Out-String | Write-Verbose

Copy link
Copy Markdown
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.

@bergmeister bergmeister Sep 13, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

@adityapatwardhan adityapatwardhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@TravisEz13

TravisEz13 commented Sep 14, 2017

Copy link
Copy Markdown
Member

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.

@TravisEz13 TravisEz13 left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.