You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/07/07 12:17:02 UTC
[GitHub] [maven-shared-utils] guylabs opened a new pull request #93: Make uninstall resilient to Jansi errors
guylabs opened a new pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93
See https://issues.apache.org/jira/browse/MSHARED-993 for details.
There is no unit/integration test for this as it would require to use Powermock or similar to mock Jansi. If requested, I can add this but I thought that this would be an overkill.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] rfscholte commented on pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#issuecomment-879665376
> > We're still trying to fix this at JAnsi. This should only be accepted if JAnsi explcitly states they won't fix it.
>
> There are still no reactions to [fusesource/jansi#214](https://github.com/fusesource/jansi/issues/214). Not sure what the timeline is for the proper fix. Do you maybe have any other insights @rfscholte ?
We're working on it as we have more ways to get direct contact with the maintainer(s). But I agree, it takes more time, must be the summer ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] guylabs commented on a change in pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
guylabs commented on a change in pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#discussion_r669298046
##########
File path: src/main/java/org/apache/maven/shared/utils/logging/MessageUtils.java
##########
@@ -98,7 +98,14 @@ private static void doSystemUninstall()
{
if ( JANSI )
{
- AnsiConsole.systemUninstall();
+ try
+ {
+ AnsiConsole.systemUninstall();
+ }
+ catch ( RuntimeException ex )
Review comment:
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] rfscholte commented on pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#issuecomment-886244198
Yes, I'll close them both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] guylabs commented on pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
guylabs commented on pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#issuecomment-886240056
@rfscholte Ok to close this pull request as https://github.com/fusesource/jansi/issues/214 has been fixed now ? Should we then also close https://issues.apache.org/jira/browse/MSHARED-993 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] guylabs commented on a change in pull request #93: Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
guylabs commented on a change in pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#discussion_r665333009
##########
File path: src/main/java/org/apache/maven/shared/utils/logging/MessageUtils.java
##########
@@ -98,7 +98,14 @@ private static void doSystemUninstall()
{
if ( JANSI )
{
- AnsiConsole.systemUninstall();
+ try
Review comment:
This was already proposed here https://issues.apache.org/jira/browse/MNG-7161 but it seemed that the consensus was to go with that approach, which makes `MessageUtils` less dependent on changes on Jansi which caused this error. Fixing the root cause will fix _this_ specific use case, but there may be others which will break too, and as the `systemUninstall` is called as a last step before Maven finishes the build we can assume that it's safe to swallow these errors. I created https://github.com/fusesource/jansi/issues/214 to fix the root cause.
IMO we should not add it to the `systemInstall` as there you'd like to be aware when we can't install the color support and get the exception during runtime, as if we would swallow that error too, it will be harder to debug the root cause without any exception thrown. If you prefer to have the same handling, then I can add this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] elharo commented on a change in pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#discussion_r669167357
##########
File path: src/main/java/org/apache/maven/shared/utils/logging/MessageUtils.java
##########
@@ -98,7 +98,14 @@ private static void doSystemUninstall()
{
if ( JANSI )
{
- AnsiConsole.systemUninstall();
+ try
+ {
+ AnsiConsole.systemUninstall();
+ }
+ catch ( RuntimeException ex )
Review comment:
does it throw a specific runtime exception?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] guylabs commented on pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
guylabs commented on pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#issuecomment-879445223
> We're still trying to fix this at JAnsi. This should only be accepted if JAnsi explcitly states they won't fix it.
There are still no reactions to https://github.com/fusesource/jansi/issues/214. Not sure what the timeline is for the proper fix. Do you maybe have any other insights @rfscholte ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] guylabs commented on a change in pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
guylabs commented on a change in pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#discussion_r669131813
##########
File path: src/main/java/org/apache/maven/shared/utils/logging/MessageUtils.java
##########
@@ -98,7 +98,14 @@ private static void doSystemUninstall()
{
if ( JANSI )
{
- AnsiConsole.systemUninstall();
+ try
+ {
+ AnsiConsole.systemUninstall();
+ }
+ catch ( Throwable ex )
Review comment:
Thanks! Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] elharo commented on a change in pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#discussion_r668738184
##########
File path: src/main/java/org/apache/maven/shared/utils/logging/MessageUtils.java
##########
@@ -98,7 +98,14 @@ private static void doSystemUninstall()
{
if ( JANSI )
{
- AnsiConsole.systemUninstall();
+ try
+ {
+ AnsiConsole.systemUninstall();
+ }
+ catch ( Throwable ex )
Review comment:
never catch Throwable. See Effective Java for reasons.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] rfscholte commented on a change in pull request #93: Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93#discussion_r665316955
##########
File path: src/main/java/org/apache/maven/shared/utils/logging/MessageUtils.java
##########
@@ -98,7 +98,14 @@ private static void doSystemUninstall()
{
if ( JANSI )
{
- AnsiConsole.systemUninstall();
+ try
Review comment:
Why is there a try-catch block required here, while it is not there for `AnsiConsole.systemInstall()`.
If this is an issue, it should be fixed in JAnsi (fix it at the root, instead of handling the symptom)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] rfscholte closed pull request #93: [MSHARED-993] Make uninstall resilient to Jansi errors
Posted by GitBox <gi...@apache.org>.
rfscholte closed pull request #93:
URL: https://github.com/apache/maven-shared-utils/pull/93
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org