fatih / color

Color package for Go (golang)
https://pkg.go.dev/github.com/fatih/color
MIT License
7.26k stars 615 forks source link

it does't working on window7(64bit) #57

Closed liyu4 closed 7 years ago

liyu4 commented 7 years ago

I get wrong when run pure-go on windows.

package main

import("github.com/fatih/color")

func main() { color.Red("we are red") }

1: window7 64bit 2: git bash

output: image

fatih commented 7 years ago

@mattn do you know what the problem might be ?

mattn commented 7 years ago

@liyu4 you use the command on cygwin or msys2 shell. it's not real command prompt. It's pseudo console. https://github.com/mattn/go-isatty is checking the handle of stdin is whether is tty or not. To recognize cygwin/msys console, go-isatty should check pipe name of handle. Fortunately, implementation alredy exists.

https://github.com/k-takata/go-iscygpty

Adding new depenency effect to too many 3rd party currently. fatih/color is depended on 458 packages. go-isatty is depended on 139 packages. So I will add new API to check both terminal using some of code from go-iscygpty. Few hours ago, I got agreement to use some of code for go-isatty from the author. Thanks @k-takata

liyu4 commented 7 years ago

well ,thanks @mattn

fatih commented 7 years ago

that's awesome. Thanks a lot @mattn for the clarification. When do you think it would be included into go-isatty? I think we can close this when you finish it.

mattn commented 7 years ago

Will do soon https://github.com/mattn/go-isatty/issues/8

mattn commented 7 years ago

Please note that I will add new API IsCygwinTerminal. I don't want to break behavior of IsTerminal. So you must change IsTerminal(fd) to IsTerminal(fd) || IsCygwinTerminal(fd).

fatih commented 7 years ago

Hm, than it would also break my code if the user doesn't have new go-isatty. Is it an interface? Maybe I can do a type check and run a function before checking it.

mattn commented 7 years ago

https://godoc.org/github.com/mattn/go-isatty

go-isatty can't add new feature without add new func at least. You must call like

https://github.com/fatih/color/blob/5ec5d9d3c2cf82e9688b34e9bc27a94d616a7193/color.go#L20

NoColor = (!isatty.IsTerminal(os.Stdout.Fd()) && !isatty.IsCygwinTerminal(os.Stdout.Fd())) || os.Getenv("TERM") == "dumb"
mattn commented 7 years ago

https://github.com/mattn/go-isatty/pull/13

mattn commented 7 years ago

Now added IsCygwinTerminal.

fatih commented 7 years ago

Thanks @mattn, I'll check how I can integrate without breaking it for older version of color.

mattn commented 7 years ago

Most easy way to do it is put latest github.com/mattn/go-isatty into your vendor directory.

fatih commented 7 years ago

Hm yeah that would work, I was against vendoring though because fatih/color is a library as well. I only vendor if it's main package, say CLI or service. But I think this is the only option I can do for now.

fatih commented 7 years ago

Waiting for: https://github.com/mattn/go-isatty/pull/14 After that I need to re-vendor again the latest.

fatih commented 7 years ago

@liyu4 can you test please this PR: https://github.com/fatih/color/pull/61 and let me know if it works or not

liyu4 commented 7 years ago

hm, hold on!

liyu4 commented 7 years ago

@fatih 😫 it's still so

below is code run on window7: image

Run: image

mattn commented 7 years ago

try to go run -x main.go

liyu4 commented 7 years ago

I have tried it like you say, but there is no effect.

this is working.

image

liyu4 commented 7 years ago

@fatih @mattn image

fatih commented 7 years ago

@liyu4 I've pushed another fix, can you please try again? I think it was due not OR'in it.

liyu4 commented 7 years ago

@fatih very pleasure!

liyu4 commented 7 years ago

@fatih 😭 i add below code to color.go NoColor = os.Getenv("TERM") == "dumb" || !isatty.IsTerminal(os.Stdout.Fd()) || !isatty.IsCygwinTerminal(os.Stdout.Fd())

but it doesn't working!

mattn commented 7 years ago

Are you really trying fix-windows branch?

fatih commented 7 years ago

@liyu4 we pushed another fix, please try again. Unfortunately I don't have the environment to test, so I'm just doing what you and @mattn and @k-takata suggest

mattn commented 7 years ago

colors are disabled in last fix.

diff --git a/color.go b/color.go
index d9f6aa1..06097b4 100644
--- a/color.go
+++ b/color.go
@@ -18,8 +18,7 @@ var (
        // or not. This is a global option and affects all colors. For more control
        // over each color block use the methods DisableColor() individually.
        NoColor = os.Getenv("TERM") == "dumb" ||
-               !isatty.IsTerminal(os.Stdout.Fd()) ||
-               !isatty.IsCygwinTerminal(os.Stdout.Fd())
+               (!isatty.IsTerminal(os.Stdout.Fd()) || !isatty.IsCygwinTerminal(os.Stdout.Fd()))

        // Output defines the standard output of the print functions. By default
        // os.Stdout is used.

This should be

    NoColor = os.Getenv("TERM") == "dumb" ||
        (!isatty.IsTerminal(os.Stdout.Fd()) && !isatty.IsCygwinTerminal(os.Stdout.Fd()))
liyu4 commented 7 years ago

sorry! I had checkout to fix-windows image

liyu4 commented 7 years ago

@mattn you're right,

that's working in my terminal, thanks @mattn @fatih

image