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

Improve log integration with AppVeyor #1490

Merged
merged 4 commits into from Apr 9, 2017

Conversation

csmager
Copy link
Contributor

@csmager csmager commented Mar 13, 2017

There were a couple of fairly inconsequential and primarily cosmetic issues that I'd noticed when AppVeyor runs a FAKE build script.

At the start, FAKE calls PrintDependencyGraph. This is written as multiple LogMessages, which the AppVeyor helper also writes to AppVeyor's message log in the Messages tab. There are two issues that arise from this:

  1. Adding a message to AppVeyor seems to be processed asynchronously. As a result, there is no ordering guaranteed. Here's a snippet from a log for a failed build:

image

  1. The call to appveyor AddMessage is included in a trace. This seems to be a little redundant:

image

  1. The call to appveyor AddMessage fails when the message is empty:

image

  1. The console output from FAKE is redirected by AppVeyor and logged. This loses all the colouring, so the log appears entirely in white. However, AppVeyor supports ANSI color codes.

The changes I've made address each of those points:

  1. I've changed the dependency message to be written as a single multi-line message so this appears as a single message in the log on AppVeyor. This makes no difference when written to the console.

  2. I've changed to disable tracing when sending a message to the AppVeyor message log.

  3. I've changed to not send empty messages to the AppVeyor message log, avoiding the error that results.

  4. I've added ANSI color code support and use this when we're running on AppVeyor. If other build servers have the same issue & support ANSI colors then perhaps this could be enabled for those too. I ran a quick test for all ConsoleColor values to show how these are rendered on AppVeyor:

image

@csmager csmager force-pushed the appveyor-logging branch 2 times, most recently from 5da43e9 to 9e0fc0e Compare March 13, 2017 11:19
@forki
Copy link
Member

forki commented Mar 13, 2017

build is red. can you please check?

@csmager
Copy link
Contributor Author

csmager commented Mar 13, 2017

@forki just looking at it now. I had tested the build on one of my projects and it works as I'd expect.

The error is in one of the tests and relates to an invalid char - the char is part of the escape sequence for ANSI color codes. As they tests are running on AppVeyor, I guess buildServer will be set to AppVeyor. It'll probably have something to do with that.

MSpec outputs the results to HTML/XML and 0x1B is not a valid character
@csmager
Copy link
Contributor Author

csmager commented Mar 13, 2017

@forki Issue seemed to be that because we were essentially bootstrapping FAKE in the test, it determined we were running AppVeyor. Because of that, the ANSI color codes were written to the trace (per my change). MSpec reports all the test results to HTML, and the escape sequence contains a character that's not valid in HTML.

I changed to essentially toggle that option off in the class that contained the failing spec in an Establish function. I've not used MSpec before, so there may be a more appropriate way of doing that.

@csmager
Copy link
Contributor Author

csmager commented Mar 13, 2017

Eurgh, my VS code settings have trimmed a load of whitespace in the files I've edited. If you want me to revert those, just shout.

@forki
Copy link
Member

forki commented Apr 9, 2017

thanks!

@forki forki merged commit 8e8098c into fsprojects:master Apr 9, 2017
BlythMeister added a commit to BlythMeister/FAKE that referenced this pull request Apr 11, 2017
This should make logging work on appveyor
forki added a commit that referenced this pull request Apr 11, 2017
Update the PrintRunningOrder to mirror logic in #1490
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.

None yet

2 participants