Check if the output supports colors (fixes #244)

[michalt]

This avoids using colors when the output is, e.g., redirected to a
file. This requried a change to avoid passing the --colour flag to
shake (so that hadrian is in charge of colors).

I've also made minor changes to the colors themselves, opting to use
the standard colors (not the bright/vivid ones, otherwise we risk that
for some configurations this will be less readable).

Finally, I've changed the color for putBuild - using white
foreground color is problematic (this was not a problem before due to
passing the --color flag to shake that was forcing different color).

Signed-off-by: Michal Terepeta michal.terepeta@gmail.com

[snowleopard]

Many thanks for picking this up, @michalt!

The current colouring scheme (let's call it palette) is important for me as I routinely have to look for specific things in build progress info. That is why putBuild needs to be brighter than standard output of build tools -- to stand out; same for putOracle, putSuccess, etc.

I think we could add a command line option to control the palette with settings like vivid (corresponding to my palette, currently the default one) and normal (similar to your palette but modified to distinguish putBuild and putOracle), and perhaps also none. We may later change the default one to normal when/if this is a popular request. We already have --progress-info, so perhaps the new option could be called --progress-palette.

We could also allow users to override the default palette in src/Settings/User.hs, as I suppose most users would prefer to set it once and then forget about this setting.

Could you modify the PR to preserve the current defaults, but keeping the hSupportsANSI check? Feel free to also implement --progress-palette if you like this idea. You'll need to modify src/CmdLineFlag.hs.

[michalt]

@snowleopard Sounds good. Let's keep this PR focused on fixing the hSupportsANSI - I don't feel that strongly about the colors. :-)

I've updated the commit and changed Dull to Vivid. I've kept the change to putBuild - otherwise the output is completely unreadable on terminals with light background (and previously was blue only because of the flag for shake). Is that ok with you?

[snowleopard]

Agreed, let's keep this PR focused on fixing the hSupportsANSI.

I've kept the change to putBuild - otherwise the output is completely unreadable on terminals with light background (and previously was blue only because of the flag for shake). Is that ok with you?

Actually this is hard to read on my terminal. I think no single palette will work well for everyone, so we will need to think this through in a separate issue. Let's drop this change for now.

More importantly, I have the following issues when trying on my usual Windows/MSYS2 set up:

• I no longer have coloured output by default when running build.bat. To get colours back I need to pass --colour to the build script, which is very inconvenient.
• When I do provide --colour flag, all Hadrian's output is blue! It doesn't matter whether it is putBuild or putOracle or putSuccess (even after restoring putBuild = putColoured White). I've no idea why, but we need to fix this.

[snowleopard]

My guess is that hSupportsANSI could be somehow broken on Windows?

[snowleopard]

Right, the blue colour now makes sense -- I guess Shake forces it this way when using --colour. And you did mention this as the reason for dropping --colour.

So, we do need to tweak the current hSupportsANSI check for it to work consistently on Windows too.

[Mathnerd314]

You could also modify Shake to use ansi-terminal (currently it just uses ANSI escapes directly: ndmitchell/shake#49)

[michalt]

Right, if you pass --colour to shake, the setSGR calls in Base.hs are simply no-ops and can be probably removed (this took me a few minutes to figure out ;-)

Looking at hSupportsANSI is pretty simple and I'm not sure what's there to fix [1]:

hSupportsANSI :: Handle -> IO Bool
-- Borrowed from an HSpec patch by Simon Hengel
-- (https://github.com/hspec/hspec/commit/d932f03317e0e2bd08c85b23903fb8616ae642bd)
hSupportsANSI h = (&&) <$> hIsTerminalDevice h <*> (not <$> isDumb)
  where
    -- cannot use lookupEnv since it only appeared in GHC 7.6
    isDumb = maybe False (== "dumb") . lookup "TERM" <$> getEnvironment

I've added a hack to always use the colors for cygwin/mingw - does this work for you? (I don't have a Windows machine to test this). Ideas for nicer alternatives are really welcome! ;-)

Finally, as requested, I've reverted the change for the putBuild to use white color as foreground. But this makes the output completely invisible (and thus useless) for me and probably many other people with terminals that have light background. See:

This was previously blue for me (since shake was overriding the white settings), so I'm not sure why this is changing for you. Are you sure this is not due to the Vivid option (I believe shake was overriding it with non-vivid colors, which corresponds to my initial change to Dull to try to preserve the old behavior).

[1] https://github.com/feuerbach/ansi-terminal/blob/bf20093f7b0fd5961cdba57d9d86bcd815352ecc/includes/Common-Include.hs#L140

[snowleopard]

Finally, as requested, I've reverted the change for the putBuild to use white color as foreground. But this makes the output completely invisible (and thus useless) for me and probably many other people with terminals that have light background.

@michalt Sure, I understand your problem and we'll fix it. I just don't want to mix it into this PR. You can use whatever colours work for you locally as a temporary workaround, but we'll provide a nicer way to do that. I think making this into a Settings/User.hs setting is the right way to go.

Now, coming back to hSupportsANSI: I don't like this System.Info hack and I am pretty sure it will bite us back if we leave it as is. I think we shouldn't go for a magic but broken solution, but instead allow users to explicitly switch colours on or off. Simple and robust.

To sum up, I am fine to merge this, but most likely I will drop the magic/ugly hack soon. Let me know if you have any other suggestions.

@Mathnerd314 Sure, fixing this in Shake would be great too, but we probably don't want to wait for the next Shake's release.

[michalt]

@snowleopard Sounds good - let's merge this and then work on removing the hack and improving the colors. Wrt. hSupportsANSI we could go for something similar to what grep is doing - it has three options for --color: never, always and auto (in our case auto could be based on the hSupportsANSI and the two first options would not use any checks).

[snowleopard]

@michalt Good idea! We can do that indeed.

This is now merged. I suggest to continue further discussions in #244.

[ndmitchell]

@snowleopard I don't think you are going to be able to just rely on Shake to magically fix it for you. Shake isn't going to provide nice and clean ANSI functions for you, you'll have to do that yourself.

[Mathnerd314]

@Mathnerd314 Sure, fixing this in Shake would be great too, but we probably don't want to wait for the next Shake's release.

I meant overriding shakeOutput in shakeOptions, which I believe is the preferred method for telling Shake how to use colors. My guess is the weird behavior with cygwin / msys is caused by a conflict between Shake's (ANSI) colors and ansi-terminal's SetSGR calls.

[ndmitchell]

That makes a lot of sense, overriding shakeOutput is a good idea if you have your own notion of what colors to use and when.

[snowleopard]

@Mathnerd314 @ndmitchell Thanks for clarifying, I will look into this!