search

snowleopard / hadrian

Hadrian: a new build system for the Glasgow Haskell Compiler. Now merged into the GHC tree!
https://gitlab.haskell.org/ghc/ghc/tree/master/hadrian
MIT License
208 stars 37 forks source link

Fix broken colours with `-j` #484

Closed patrickdoc closed 6 years ago

patrickdoc commented 6 years ago

Doing the high-impact work, I have fixed the colored output when running with -j :P This patch also removes ansi-terminal as a dependency, which will be easier to explain in a second.

The root of the issue is that hadrian currently outputs:

\ESC[34m<SOME OUTPUT>\n
\ESC[0m

with newline character added for emphasis. My brief research suggests stdout is typically line buffered, so the final reset code can break other lines.

If you chase Shake's definition of putNormal all the way to https://github.com/ndmitchell/shake/blob/c9c4246ec6e9d3ca80eabea10d64e01b6c70d094/src/Development/Shake/Internal/Core/Run.hs#L75, you can see that it uses locks. So if we can combine the color and message output into a single putNormal call, we can drop a fair bit of the logic.

This means we have to manually construct the color codes, but that is fairly easy. The setSGR code that was being used boils down to: hPutStr "\ESC[<color>m". It only takes two small data types and some helper functions to do that in this code. We also get access to the extended 256 color set with ~2 lines of extra code, for output color connoisseurs.

Shake actually uses a simplified version of this internally here, https://github.com/ndmitchell/shake/blob/be4333474930ff30c7232894be66bdbb93f01702/src/Development/Shake/Internal/Args.hs#L244.

After taking that functionality from ansi-terminal, the only use left was hSupportsANSI. That code is a two-liner borrowed from HSpec, https://github.com/hspec/hspec/commit/d932f03317e0e2bd08c85b23903fb8616ae642bd. Recreating that in this code is also fairly easy.

I get that removing a dependency is a little aggressive, so if you want me to tone is down I can.

As for testing, I ran a full build on Debian and Mac with no errors. I am not brave enough to do anything on Windows. It is also a little hard to test these kinds of bugs; but almost all of the responsibility is on Shake now, so it makes our lives easier.

snowleopard commented 6 years ago

@patrickdoc Awesome, thank you! I've always wanted to fix broken colours in -j mode, but I never considered reimplementing ansi-terminal from scratch :) The result doesn't look too bad in terms of the amount of new code.

Your PR passed CI, so I think I'll merge this if there are no objections. I might do some minor revision later.

@bgamari @ndmitchell Any comments on this?

snowleopard commented 6 years ago

@bgamari @ndmitchell I am planning to merge this tomorrow. Please shout if you have any concerns.

snowleopard commented 6 years ago

@patrickdoc Thank you, this is now merged!