-
Notifications
You must be signed in to change notification settings - Fork 109
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
Force maven to output color #873
Conversation
@mihnita as mentioned in the recent discussion, I think there should be a way for to programmatically query if a console will support ANSI colors, for example like this in the UI: then M2E should query the flag and act accordingly to a new User setting drop-down: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mihnita. That's a good location. 👍🏽
But as @laeubi said, we should not always add that flag. Otherwise, in case the coloring is disabled, we might see all the ANSI escape-sequences and we don't want that.
@laeubi do we really need that configurable? Simplify enabling that flag when the console is colored seems sufficient to me? I cannot imagine that one wants to see the escape-characters and having black-and-white Maven printout if console colors are enabled also seems irrelevant to me. And it would save us another preference.
Besides that I wonder if the VM arguments -Djansi.force=true
and -Djansi.passthrough=true
are necessary? At least the m2e-chromatic extension has it? Or is this obsolete with the integration into Eclipse?
https://github.com/sidespin/m2e-chromatic/blob/master/m2e.chromatic.core/src/main/java/io/sidespin/eclipse/m2e/chromatic/internal/ColoredMavenLaunchParticipant.java
Another thing we have to keep in mind is that the MavenConsoleLineTracker
has to be adjusted to consider the ANSI escape-sequences when creating links to the project etc. I have done that a while ago already but back then coloring support was only possible with external plug-ins and we didn't want to provide support for external plug-ins.
It was #535. When reading through it, I just noticed you know that already. A lot has changed since then :D So I think I will revive that PR to address the handling of escaped printout in the Tracker. After this is done. But I wonder if there is now a better way to do what was done there. But lets discuss this there.
Thank you @laeubi Adding the Auto / Never / Always options is easy (working on it) But there is a problem with the support per console. So right now I can't think of a way to query if a console supports ANSI colors or not. Some options for this feature (enable colors for maven output):
We do nothing for 2022-09. People can still define the parameter Pros: zero work, we don't shake the boat so close to a release, and we have time to get it right the next release.
Pros: not intrusive at all. Basically one UI widget + the current code + one if statement
That way we can check the status and implement Auto. Cons: the most work, close to a release. I don't have a strong opinion, but I will go the option 2 and update this PR with that, to save time if we go in that direction. |
Btw. did you manage to solve your problems with the m2e setup you mentioned in eclipse-platform/eclipse.platform.debug#50 (comment)? |
Yes, I did :-)
|
I am looking into the build failures. |
|
Yes probably something like in #535 has to be done. I'll look into it tomorrow it is late now where I live. :)
Have you seen this question from me? Maybe it was list between the other ones. |
The other failures are in But the patch would be something like this:
|
And to confirm: |
At least maven allows to configure that: https://issues.apache.org/jira/browse/MNG-6220 with the options
Who knows? At least users can't complain that we force them to see all these fancy colors :-)
I think it would be useful to adjust that, but that's just for the next release maybe but still something to keep in mind any maybe prepare right now.
The ANIS-Console stores it somewhere, and m2e could read it from the "somewhere" until there is a better choice.
I think unless we can know that ANSI is really enabled we should default to not enabling colours, Beside that you can:
|
It is stored in the preferences. I'll give it a try though. |
Added support for auto, reading the preferences. Disclaimer I was unable to test the functionality end to end, as the Ansi support is in I started with a new installer and cloned both repos, but I was unable to build them together without errors. So I did it "by pieces"
|
I managed to get both
I've also created a PR for Thanks, |
You can modify M2E's target platform to include the 4.25-I-builds repo that contain the master from the night of o.e.eclipse.debug.
Thanks for confirming. But it looks like the org.fusesource.jansi:jansi is still required in the maven-runtime. To bad I hoped we could drop that because it causes extra effort in the build (the native libs are not Mac-signed). Do you know why this is still required? Or did I test it wrong? I will have a look at the test on Thursday evening. Sorry for the delay but this weekend I wanted to complete a new feature for PDE that has to be completed until tomorrow evening. |
you can update the
Just push the updated file to this PR and Jenkins should pick that up. |
Any idea why the coloring is different? |
I think this can be investigated independent of this change but maybe:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor remark.
org.eclipse.m2e.core.ui.tests/src/org/eclipse/m2e/core/ui/tests/ConsoleTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.launching/src/org/eclipse/m2e/internal/launch/MavenLaunchDelegate.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.launching/src/org/eclipse/m2e/ui/internal/launch/MavenLaunchMainTab.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.launching/src/org/eclipse/m2e/internal/launch/ColorOutput.java
Outdated
Show resolved
Hide resolved
From all I know, jansi does a couple of things:
Functionality 2 and 3 require native code. This documents what I don't know how Maven uses jansi. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Not really... |
I think we should first finish this PR so not mixing too much concerns :-) |
Sorry, I don't manage to make the |
Agree :-) |
I'm trying to go through the blockers for this PR. GitHub complains about unaddressed change request. When I click "See review" it takes me to this comment:
I think it rises a few items:
And the fixes for the test failures in So maybe you can just merge them "almost at the same time?" :-) Or clone my 2 PRs and create one, as you seem to know how, but I don't manage to do. Last: where can I update the help? Not blocker for this PR, but needs to be done. Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mihnita, yes all previous remarks are addressed or should/can be addressed separately.
I did a deeper review and found one point that probably has to be corrected. See the attached review.
I also looked into to tests and tried them. They look good. 👍🏽 I will submit them close before this PR is about to be submitted.
Can you also please squash the changes of this PR into two commits:
Changed threads label
-commit just as it is (you can use git's interactive rebase to reorder commits)- All the changes for the coloring support squashed into one with an message+description that covers the commit as a whole (e.g. version bumps don't have to be mentioned in the squashed commit message).
org.eclipse.m2e.launching/src/org/eclipse/m2e/internal/launch/MavenLaunchDelegate.java
Show resolved
Hide resolved
org.eclipse.m2e.launching/src/org/eclipse/m2e/internal/launch/MavenLaunchDelegate.java
Outdated
Show resolved
Hide resolved
I've explained why If you agree with that, then the commits are merged and ready to submit. If you have an idea how to handle that, then I'm listening :-) My ideas:
And Thanks, |
OK, I can't find a way to detect ANSI support. But I've tried, and something like this would work:
Would need to only do it once and cache the result, and the documentation for What do you think? |
I think that looks like a very good "hack" to the current problem and the performance penalty is really something we can ignore here. We do not expect user to fire up maven builds in the range of thousands or more withing seconds ... |
With UI to make color output optional and with support for `auto / always / never` (auto : color if ANSI is enabled)
Added It is almost 100% what I described before, but added a workaround for the case when one is developing and the qualifier of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
isAnsiSupportInstalled()
, line 346 ofMavenLaunchDelegate.java
It is almost 100% what I described before, but added a workaround for the case when one is developing and the qualifier of
org.eclipse.ui.console
is "qualifier". That only makes a difference if the version is3.11.300.qualifier
, otherwisecompareTo
does not even get to the qualifier part. Should be very rare. But it affected me :-), so it is not impossible that others might also be affected.
Agree that this is looks good for now. Platform.getBundle(String)
is not the fastest thing on earth but also not the slowest and given that it is only called once per launch nobody will notice it.
However I consider to build M2E against the 4.25-I-builds soon (maybe even this evening). Then we could simply lift the minimum required version of the console bundle and just remove that 'hack. Because as you asked, I don't think it is possible to install M2E without it being present.
If we release M2E before the Eclipse 2022-09 release then it is definitely targeted for that release so that should be fine to have requirements on that. Maybe we even have some more connectors adapted to M2E 2.0, but that is another story.
I have merged to adjustments of the m2e-core-tests
and started a fresh build that should automatically pick them up. If that is green, this is ready to go.
Thanks again, @mihnita!
Jenkins Build is green (GH-build does not automatically fetch the latest submodule master and therefore fails, but that's ok). So this is ready. Thank you @mihnita, great work! |
This is a minimal change, but to me it feels very brute force and ugly.
Is this the right way?
And do we want to expose some user preference so that they might disable it?
Maybe some users have some kind of automation monitoring the console looking for stuff, and this might break it.
@HannesWell
What do you think?
Thank you,
Mihai