Leverage Maven's -Dstyle.color to avoid coloring instead of stripping the ASCII codes in the client

[ppalaga]

Fix #373

[gnodet]

I don't think leveraging the somewhat broken (atm) style.color option is a good idea, see for example https://issues.apache.org/jira/browse/MNG-6380?filter=12350130. I've raised a bunch of PR in maven to fix the coloring, but I don't think they'll be integrated in 3.x branch, but rather 4.x. And I don't really want to wait for maven 4.x for those fixes in mvnd.
Also, tying the no-color option to non-buffering does not seem a benefit. The default buffering mode allows to read the output much more easily (per project logs are grouped together).
What's the problem with the current approach ?

[ppalaga]

I don't think leveraging the somewhat broken (atm) style.color option is a good idea, see for example https://issues.apache.org/jira/browse/MNG-6380?filter=12350130.

With my current proposal, mvnd -B -Dstyle.color=always package | less -R outputs colors, as expected. Explicit -Dstyle.color is always honored, and -B does not override it in any way.

I've raised a bunch of PR in maven to fix the coloring, but I don't think they'll be integrated in 3.x branch, but rather 4.x. And I don't really want to wait for maven 4.x for those fixes in mvnd.
Also, tying the no-color option to non-buffering does not seem a benefit.

No, I do not tie no-color to noBuffering. You probably saw this location https://github.com/mvndaemon/mvnd/pull/371/files#diff-2e0b63101a0e22feb7df48ea1a5e83e867c4d076bb72ea4eeccfc5a79dca756bR117 but there, noBuffering is i.a. triggered by !stdoutIsTerminal which IMO is the right thing to do. I admit, it might have been a separate commit, because it is not directly related to coloring. I am open to discuss it.

The default buffering mode allows to read the output much more easily (per project logs are grouped together).

I agree, and do not think I did anything against the benefits the default buffering mode brings.

What's the problem with the current approach ?

Originally, I had two motivations for these changes (and when implementing another two appeared):

1. I did not like that we add the new --color option for something that was solvable by available Maven options. I originally thought we could use -B under the hood (and then learned about -Dstyle.color which is better). Now that I know that --color is proposed for Maven 4, it is actually quite acceptable and if you like, I would not mind adding --color back (keeping -Dstyle.color as its alias).

2. I found the state after Fix color output for file / tty #358 #359 a bit ineffective: we instruct the daemon to produce colored messages and then we strip colors in the client https://github.com/mvndaemon/mvnd/pull/359/files#diff-3bfeaf9064d5d02331090c71be3e33960138f55d4fc7cc11fb692a587b30cb91R842 Isn't it more effective to tell the daemon whether we want colored messages upfront using -Dstyle.color=always or -Dstyle.color=never? You seem to imply that -Dstyle.color does not work properly. You have pointed to an example with -B and -Dstyle.color=always combined, but as I mentioned above, that case works well with the current proposal, IMO. Maybe you know more situations where -Dstyle.color does not work as expected?

3. I have found a related bug that existed both before and after Fix color output for file / tty #358 #359 : Once the daemon was started with -B option, all subsequent invocations, albeit without -B, were colorless. The current PR solves this problem by always sending an explicit -Dstyle.color=always or -Dstyle.color=never to the daemon.

4. As noted above, !stdoutIsTerminal should trigger noBuffering.

Points 3. and 4. are separate problems and I would not mind discussing them separately, after we find an agreement about 1. and 2.

[gnodet]

1. I did not like that we add the new --color option for something that was solvable by available Maven options. I originally thought we could use -B under the hood (and then learned about -Dstyle.color which is better). Now that I know that --color is proposed for Maven 4, it is actually quite acceptable and if you like, I would not mind adding --color back (keeping -Dstyle.color as its alias).

Ok. I'd rather keep the --color option as an alias.

2. I found the state after Fix color output for file / tty #358 #359 a bit ineffective: we instruct the daemon to produce colored messages and then we strip colors in the client https://github.com/mvndaemon/mvnd/pull/359/files#diff-3bfeaf9064d5d02331090c71be3e33960138f55d4fc7cc11fb692a587b30cb91R842 Isn't it more effective to tell the daemon whether we want colored messages upfront using -Dstyle.color=always or -Dstyle.color=never? You seem to imply that -Dstyle.color does not work properly. You have pointed to an example with -B and -Dstyle.color=always combined, but as I mentioned above, that case works well with the current proposal, IMO. Maybe you know more situations where -Dstyle.color does not work as expected?

We'd have to verify the behavior of MNG-6719

3. I have found a related bug that existed both before and after Fix color output for file / tty #358 #359 : Once the daemon was started with -B option, all subsequent invocations, albeit without -B, were colorless. The current PR solves this problem by always sending an explicit -Dstyle.color=always or -Dstyle.color=never to the daemon.

Yes, that one should be fixed. My idea was to always have coloring on the server side, but it's true that unless the system property is always set explicitly, it could keep the previous values, which is wrong.

4. As noted above, !stdoutIsTerminal should trigger noBuffering.

I don't think I agree on that one. I often redirect the output to a log file, but that does not mean I want the project's output to be interleaved.

Points 3. and 4. are separate problems and I would not mind discussing them separately, after we find an agreement about 1. and 2.

Can we move #4 out. Also, I think constants would be nice to have for always, never and auto values.

[ppalaga]

1. I did not like that we add the new --color option for something that was solvable by available Maven options. I originally thought we could use -B under the hood (and then learned about -Dstyle.color which is better). Now that I know that --color is proposed for Maven 4, it is actually quite acceptable and if you like, I would not mind adding --color back (keeping -Dstyle.color as its alias).

Ok. I'd rather keep the --color option as an alias.

OK, let me add it back.

2. I found the state after Fix color output for file / tty #358 #359 a bit ineffective: we instruct the daemon to produce colored messages and then we strip colors in the client https://github.com/mvndaemon/mvnd/pull/359/files#diff-3bfeaf9064d5d02331090c71be3e33960138f55d4fc7cc11fb692a587b30cb91R842 Isn't it more effective to tell the daemon whether we want colored messages upfront using -Dstyle.color=always or -Dstyle.color=never? You seem to imply that -Dstyle.color does not work properly. You have pointed to an example with -B and -Dstyle.color=always combined, but as I mentioned above, that case works well with the current proposal, IMO. Maybe you know more situations where -Dstyle.color does not work as expected?

We'd have to verify the behavior of MNG-6719

I have verified that mvn verify | tee mvn.log when executed on Windows with git (bash) does not write the color escape to the file.

3. I have found a related bug that existed both before and after Fix color output for file / tty #358 #359 : Once the daemon was started with -B option, all subsequent invocations, albeit without -B, were colorless. The current PR solves this problem by always sending an explicit -Dstyle.color=always or -Dstyle.color=never to the daemon.

Yes, that one should be fixed. My idea was to always have coloring on the server side, but it's true that unless the system property is always set explicitly, it could keep the previous values, which is wrong.

Filed #373 so that we have it in the change log.

4 As noted above, !stdoutIsTerminal should trigger noBuffering.

I don't think I agree on that one. I often redirect the output to a log file, but that does not mean I want the project's output to be interleaved.

Yes, thanks, that sounds as a valid reason. Let me remove it.

Also, I think constants would be nice to have for always, never and auto values.

+1

[ppalaga]

All done, could you please review?

[gnodet]

There's another thing that comes to my mind.

[ppalaga]

Added Thread.sleep(1) to CacheFactoryTest