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

[Jansi 1.16 / Maven 3.5.1 regression] ANSI escape codes for colors are not interpreted correctly in Cygwin and MINGW (GitBash and Intellij IDEA terminals) #94

Closed
dejan2609 opened this Issue Sep 12, 2017 · 13 comments

Comments

Projects
None yet
2 participants
@dejan2609
Contributor

dejan2609 commented Sep 12, 2017

Prologue: Maven jira ticket -->> MNG-6282

Details / history:

  • maven 3.5.0 (with jansi 1.13): everything works fine
  • maven 3.5.1 (with jansi 1.16): console output fails to show colors (red, blue, green, yellow)
    Jansi revert / downgrade (to, say, version 1.15) solves colors issues in maven 3.5.1, but, according to @hboutemy, this action is a no-go a due to related changes / tickets (see this JIRA comment)

Git-bisect shows this commit (that introduced regression): bb3d538 ANSI output stripping does not work if TERM is xterm, fixes #83

Maven 3.5.0 and 3.5.1 outputs in GitBash and Cygwin terminals:

Note: @hboutemy branch / PR #88 was used in order to confirm issues on both terminals (see comments below).

@dejan2609

This comment has been minimized.

Contributor

dejan2609 commented Sep 12, 2017

Environment details (Git-for-windows/GitBash):

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi (executable)
$ java -version
java version "1.8.0_144"
Java(TM) SE Runtime Environment (build 1.8.0_144-b01)
Java HotSpot(TM) 64-Bit Server VM (build 25.144-b01, mixed mode)

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi (executable)
$ mvn -version
Apache Maven 3.5.0 (ff8f5e7444045639af65f6095c62210b5713f426; 2017-04-03T21:39:06+02:00)
Maven home: C:\Work\tools\maven\apache-maven-3.5.0
Java version: 1.8.0_144, vendor: Oracle Corporation
Java home: C:\Program Files\Java\jdk1.8.0_144\jre
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi (executable)
$ git --version --build-options
git version 2.14.1.windows.1
built from commit: 82d9b3f3b2407b52251620597d4b14933685459d
sizeof-long: 4
machine: x86_64

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi (executable)
$ cmd.exe /c ver

Microsoft Windows [Version 10.0.15063]

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi (executable)
$ cat /etc/install-options.txt
Path Option: Cmd
SSH Option: OpenSSH
CURL Option: OpenSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Disabled

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi (executable)
$ git status --short

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi (executable)
$ git lg -3
* 257b1de - (HEAD -> executable, hboutemy/executable) added main class to jansi.jar to help diagnose issues or configs (4 months ago)<Hervé Boutemy>
* da41f4a - (origin/master, origin/HEAD, master) [maven-release-plugin] prepare for next development iteration (4 months ago)<Guillaume Nodet>
* b8d3af5 - (tag: jansi-project-1.16) [maven-release-plugin] prepare release jansi-project-1.16 (4 months ago)<Guillaume Nodet>

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi (executable)
$ cd jansi/target/

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi/jansi/target (executable)
$ java -jar jansi-1.17-SNAPSHOT.jar
Jansi 1.17-SNAPSHOT (Jansi native 1.7, HawtJNI runtime 1.15)

library.jansi.path=
library.jansi.version=

os.name= Windows 10, os.version= 10.0, os.arch= amd64
file.encoding= Cp1252
java.version= 1.8.0_144, java.vendor= Oracle Corporation, java.home= C:\Program Files\Java\jdk1.8.0_144\jre

jansi.passthrough= false
jansi.strip= false
jansi.force= false
org.fusesource.jansi.Ansi.disable= false

IS_WINDOWS= true
IS_CYGWIN= false
IS_MINGW= true

                      ??????????? ??????? ???????????
                      ????????????????????????????̦?
                 ???? ?????????????????????????? ????
                 ???????????????????? ???? ??????????
                 ???????????? ??????? ???????????????
                  ??????????? ??????? ???????????????


dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi/jansi/target (executable)
$ date
12 Sep 2017 17:25:00

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-17/other/jansi/jansi/target (executable)
@dejan2609

This comment has been minimized.

Contributor

dejan2609 commented Sep 12, 2017

Environment details (Cygwin):

dejans@BEG-IT-09 /cygdrive/c/Work/IDEA-17/other/jansi
$ cygcheck -V
cygcheck (cygwin) 2.9.0
System Checker for Cygwin
Copyright (C) 1998 - 2017 Cygwin Authors
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

dejans@BEG-IT-09 /cygdrive/c/Work/IDEA-17/other/jansi
$ cd jansi/target/

dejans@BEG-IT-09 /cygdrive/c/Work/IDEA-17/other/jansi/jansi/target
$ java -jar jansi-1.17-SNAPSHOT.jar
Jansi 1.17-SNAPSHOT (Jansi native 1.7, HawtJNI runtime 1.15)

library.jansi.path=
library.jansi.version=

os.name= Windows 10, os.version= 10.0, os.arch= amd64
file.encoding= Cp1252
java.version= 1.8.0_144, java.vendor= Oracle Corporation, java.home= C:\Program Files\Java\jdk1.8.0_144\jre

jansi.passthrough= false
jansi.strip= false
jansi.force= false
org.fusesource.jansi.Ansi.disable= false

IS_WINDOWS= true
IS_CYGWIN= true
IS_MINGW= false

                      ??????????? ??????? ???????????
                      ????????????????????????????̦?
                 ???? ?????????????????????????? ????
                 ???????????????????? ???? ??????????
                 ???????????? ??????? ???????????????
                  ??????????? ??????? ???????????????


dejans@BEG-IT-09 /cygdrive/c/Work/IDEA-17/other/jansi/jansi/target
$ date
12 Sep 2017 17:29:38

dejans@BEG-IT-09 /cygdrive/c/Work/IDEA-17/other/jansi/jansi/target
$



@dejan2609

This comment has been minimized.

Contributor

dejan2609 commented Sep 12, 2017

Quoting @hboutemy (from JIRA comment)

define with JAnsi the list of configuration that have to be checked (OS+shell type), ideally with a collection of VM images to ease reproducing (I don't know if this is feasible with Windows licensing model, but this would really be a big help)

Maybe this would be a good starting point, I'll try to dig deeper:

@hboutemy

This comment has been minimized.

Collaborator

hboutemy commented Sep 14, 2017

here are my latest findings: java -Djansi.force=true jansi-*.jar displays color
then it seems the issue is caused by isatty native call not detecting tty with GitBash: I did not investigate if isattywas working in JAnsi 1.15 or if it was just not used
I need some time to improve the test program and run tests against HEAD and 1.15: I'll do it this week end and report

@dejan2609

This comment has been minimized.

Contributor

dejan2609 commented Sep 14, 2017

Thanx @hboutemy.
I did some additional experimenting/testing; my environment is the same as noted above: Maven 3.5.1 (powered by Jansi 1.16) on Windows 10 (with Cygwin 2.9.0 and Git-for-windows 2.14.1.windows.1).

Results for different terminals:

  • cmd.exe:
  • PowerShell: (cmd. exe and powershell are not using jansi, they are listed only for a reference / comparison)
  • Eclipse MINGW (TM terminal plugin):
  • GitBash MINGW: no extra colors (red, green, blue, yellow) are shown
  • Cygwin: no extra colors are shown (same as in GitBash MINGW terminal)
  • IntelliJ IDEA MINGW: no colors are shown; also, ANSI escape sequences/codes for colors are interpreted/shown like this:
dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-2017.2/projects/jansi/jansi (executable)
$ mvn clean
[?[1;34mINFO?[m] Scanning for projects...
[?[1;34mINFO?[m]
[?[1;34mINFO?[m] ?[1m------------------------------------------------------------------------?[m
[?[1;34mINFO?[m] ?[1mBuilding jansi 1.17-SNAPSHOT?[m
[?[1;34mINFO?[m] ?[1m------------------------------------------------------------------------?[m
[?[1;34mINFO?[m]
[?[1;34mINFO?[m] ?[1m--- ?[0;32mmaven-clean-plugin:3.0.0:clean?[m ?[1m(default-clean)?[m @ ?[36mjansi?[0;1m ---?[m
[?[1;34mINFO?[m] ?[1m------------------------------------------------------------------------?[m
[?[1;34mINFO?[m] ?[1;32mBUILD SUCCESS?[m
[?[1;34mINFO?[m] ?[1m------------------------------------------------------------------------?[m
[?[1;34mINFO?[m] Total time: 0.560 s
[?[1;34mINFO?[m] Finished at: 2017-09-14T11:19:03+02:00
[?[1;34mINFO?[m] Final Memory: 9M/184M
[?[1;34mINFO?[m] ?[1m------------------------------------------------------------------------?[m

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-2017.2/projects/jansi/jansi (executable)
$ date
14 Sep 2017 11:19:08

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-2017.2/projects/jansi/jansi (executable)

Snapshots (IDEA vs. GitBash vs Eclipse)

@dejan2609 dejan2609 changed the title from Jansi 1.16 regression: Maven 3.5.1 fails to show colors in console (in both GitBash and Cygwin) to [Jansi 1.16 / Maven 3.5.1 regression] ANSI escape codes for colors are not interpreted correctly in Cygwin and MINGW (GitBash and Intellij IDEA terminals) Sep 14, 2017

@dejan2609

This comment has been minimized.

Contributor

dejan2609 commented Sep 14, 2017

@hboutemy I digged this for your isatty test (I tried it just a bit, will do some more testing tomorrow):

        int value = isatty(CLibrary.STDOUT_FILENO);
        if( value ==0 ) {
            System.out.println("stdout is a tty");
        } else {
            System.out.println("stdout is NOT a tty");
        }
@dejan2609

This comment has been minimized.

Contributor

dejan2609 commented Sep 14, 2017

It seems that current jansi HEAD recognizes GitBash standard output tty (I added check from above here: https://github.com/hboutemy/jansi/pull/1).
Update: I failed to realize that value 0 represents not associated with a terminal state...Will switch values and test again.

@dejan2609

This comment has been minimized.

Contributor

dejan2609 commented Sep 15, 2017

isatty test (https://github.com/hboutemy/jansi/pull/2) shows this:

IntelliJ IDEA MINGW64: (stdout IS a TTY, 'isatty' has a value of 64):

?[?7h?[255D?[40m
?[0;1;33m?[22C?[32m??????????? ??????? ???????????
?[22C??[37m???[32m???[37m??????[32m????[37m??????[32m????[37m???????[32m??[37m̦?[32m?
?[17C???? ??[37m???[32m??[37m????????[32m??[37m???[32m????[37m???[32m??[37m?????? ?[32m??[37m???[32m?
?[17C??[37m???[32m????[37m???[32m??[37m???[32m????[37m???[32m??[37m???[32m? ??[37m???[32m? ?[37m???????[32m??[37m???[32m?
?[17C???[37m??????[32m???[37m???[32m? ??[37m???[32m??[37m???[32m? ??[37m???[32m??[37m????????[32m??[37m???[32m?
?[18C??????????? ??????? ???????????????
?[0m

Cygwin and GitBash MINGW64 (via TM terminal plugin) (no colors): (stdout is NOT a TTY, 'isatty' has a value of 0):

                  ??????????? ??????? ???????????
                  ????????????????????????????̦?
             ???? ?????????????????????????? ????
             ???????????????????? ???? ??????????
             ???????????? ??????? ???????????????
              ??????????? ??????? ???????????????

Eclipse MINGW64 (viaTM terminal plugin) is a clear winner:

image

@dejan2609

This comment has been minimized.

Contributor

dejan2609 commented Sep 15, 2017

As stated by @hboutemy

then it seems the issue is caused by isatty native call not detecting tty with GitBash

This is likely to be the case (at least for a current jansi HEAD).
Real life example that could help: https://stackoverflow.com/questions/40912072/git-for-windows-mintty-sys-stdout-isatty-returns-false

Not sure about IntelliJ IDEA although.

@hboutemy

This comment has been minimized.

Collaborator

hboutemy commented Sep 16, 2017

I updated Jansi test program to be explicit on the choice done by Jansi: PASSTHROUGH, STRIP_ANSI, WINDOWS or RESET_ANSI_AT_CLOSE. With this update, it clearly shows that on GitBash, Jansi does STRIP_ANSI
on the cause, it looks like your pointer permits to find a workaround done in Git that Jansi should probably implement: git/git@8692483 . See the commit message for detailed explanations, but in short "MSYS2 and Cygwin emulate pseudo terminals via named pipes, meaning that isatty() returns 0"
And the fix for #83 is technically an improvement for general Jansi consistency, but just reveals that isatty never worked on MSYS32 nor Cygwin but Jansi was previously ignoring it by inconsistent behaviour between Unix (that supports ANSI escape codes) and MSYS32 or Cygwin terminal on Windows (that support ANSI escape codes also, even if on Windows...)

@hboutemy

This comment has been minimized.

Collaborator

hboutemy commented Sep 16, 2017

I have an easy half fix: just don't strip ANSI escape code when stdout is not a terminal if on MINGW or CYGWIN

            // If we can detect that stdout is not a tty then setup
            // to strip the ANSI sequences (if not MINGW or CYGWIN since their terminal is not
            // a terminal but a pseudo: see https://github.com/git/git/commit/8692483)
            if (!forceColored && isatty(fileno) == 0 && !IS_MINGW && !IS_CYGWIN) {
                jansiOutputType = JansiOutputType.STRIP_ANSI;
                return new AnsiOutputStream(stream);
            }

This is not perfect, since someone on Cygwin redirecting output to a file will get ANSI escape codes (should be stripped), but this is IMHO a minimal price to pay
Testing this little code change gives following result:

IS_WINDOWS= true
IS_CYGWIN= false
IS_MINGW= true

isatty(STDOUT_FILENO)= 0, System.out is *NOT* a terminal
isatty(STDERR_FILENO)= 0, System.err is *NOT* a terminal

Jansi System.out mode: RESET_ANSI_AT_CLOSE
Jansi System.err mode: RESET_ANSI_AT_CLOSE

as you can see, stdout is not seen as terminal (given the real workaround for MINGW pseudo terminal detection has not been implemented) but Jansi now decides to not strip ANSI codes

This can be used as a quick workaround if implementing full algorithm is too hard (I'm not able to do it myself, for example, since it will require a jansi-native update)

@hboutemy

This comment has been minimized.

Collaborator

hboutemy commented Sep 22, 2017

just tested: ttyname() method from jansi-native is not available on Windows
looks like we'll really require the NtQueryObject method that is used by git/git@8692483

        /* get pipe name */
 	if (!NT_SUCCESS(NtQueryObject(h, ObjectNameInformation,
 			buffer, sizeof(buffer) - 2, &result)))
 		return;
 	name = nameinfo->Name.Buffer;
 	name[nameinfo->Name.Length] = 0;
 
	/*
	 * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')
	 * or a cygwin pty pipe ('cygwin-XXXX-ptyN-XX')
	 */
	if ((!wcsstr(name, L"msys-") && !wcsstr(name, L"cygwin-")) ||
			!wcsstr(name, L"-pty"))
		return;
@hboutemy

This comment has been minimized.

Collaborator

hboutemy commented Feb 28, 2018

fixed by using jansi-native 1.8 in 3f47f7e

@hboutemy hboutemy closed this Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment