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

Do not assume no color on windows. #957

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

jtnord
Copy link

@jtnord jtnord commented Apr 23, 2019

recent windows 10 can support colour (it is even enabled in logrus)
however the default formatter does not use colour and so the default log
format is different on windows to other OSes when logging to the
terminal. This removed the hard code no colour for windows terminals to
be consistent with other OSes

fixes #763

recent windows 10 can support colour (it is even enabled in logrus)
however the default formatter does not use colour and so the default log
format is different on windows to other OSes when logging to the
terminal.  This removed the hard code no colour for windows terminals to
be consistent with other OSes

fixes sirupsen#763
Now that windows coloring behaves correctly the unit test for
text_formatter was incorrect.

changes the expected test output to be the same regardless of the OS.
@dgsb
Copy link
Collaborator

dgsb commented May 10, 2019

@jtnord there have been back and forth pull request on this topic.
IIRMC, the last pull request desactived coloring on windows, because color support in the terminal depending on the windows version can not been guaranteed.
That said, I'm not a windows expert at all.
Before merging that, I would like you check the last closded pull request on this topic in order to assess what has been done before.

@dgsb
Copy link
Collaborator

dgsb commented May 10, 2019

@jtnord what about this one #864 ?
@ceriath do you mind to have a look here ?

@jtnord
Copy link
Author

jtnord commented May 10, 2019

Windows 10 has according to this site more than 50% of the windows desktop market and given the updates are pushed pretty hard, and the version that has ANSI support is pretty old now it would suggest that the switch over point to flip the behaviour is here or very close. (see also this page for an old breakdown of versions

I guess whilst we can endlessly debate the use of color vs no color the issue we have is that this drives not the use of colour but the output format, and this change does not address that, it was just a simple enough fix for my needs (#763)

@ceriath
Copy link
Contributor

ceriath commented May 12, 2019

@dgsb the reason for my initial PR on this topic was - as i stated there - not that windows 10 was uncapable of supporting colours, it just was not supported by default, you had to turn it on manually (as in the thread linked by @cup). While definitely is possible to use colours, if you know what you are doing, it will not work for most users that have not explicitly enabled it.
People that know, that they enabled it can still use forceColor to have colours, but i think logrus should by default match with the default windows behaviour.

Another fix could be to just check the registry for the key, but i don't think that would be worth the effort and costs

@ghost ghost mentioned this pull request May 12, 2019
@ghost
Copy link

ghost commented May 12, 2019

After looking at this more closely I am actually a fan of this. Windows 10 ANSI
colors are disabled by default:

https://superuser.com/questions/413073/-/1300251

but they still work if the application specifies it, which Logrus does:

sequences.EnableVirtualTerminalProcessing(syscall.Handle(v.Fd()), true)

For earlier Windows versions it would help if #965 was fixed as well. Currently
with Windows 8.1 this is needed:

package main
import "github.com/mattn/go-colorable"
import "github.com/sirupsen/logrus"
func main() {
   logrus.SetFormatter(&logrus.TextFormatter{ForceColors: true})
   logrus.SetOutput(colorable.NewColorableStdout())
   logrus.Info("aaaaa bbbbb")
}

and this for Windows 10:

package main
import "github.com/sirupsen/logrus"
func main() {
   logrus.SetFormatter(&logrus.TextFormatter{ForceColors: true})
   logrus.Info("aaaaa bbbbb")
}

If this issue was fixed as well as #965, this could be used for Linux,
Windows 10 and Windows 8.1:

package main
import "github.com/sirupsen/logrus"
func main() {
   logrus.Info("aaaaa bbbbb")
}

@kevpar
Copy link

kevpar commented May 23, 2019

I didn't notice this PR already existed, but I just opened #974 which addresses the old Windows version issue by detecting when EnableVirtualTerminalProcessing fails, and falling back to no colors in that case. It should allow colors on new Windows versions without impacting output on old versions.

@stale stale bot added the stale label Feb 26, 2020
@markphelps markphelps removed the stale label Feb 26, 2020
Repository owner deleted a comment from stale bot Feb 26, 2020
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.

Default logging format different across architectures
5 participants