-
Notifications
You must be signed in to change notification settings - Fork 139
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 drowns the entire VM if platform is not supported #55
Comments
Christian Schulte prepared #54 PR to fix the issue, by adding a try/catch in init |
#54 catches UnsatisfiedLinkError and there is no info that native libraries initialization failed. |
@gslowikowski I fully agree here. Not helpful to know that native functions aren't available. |
I tried to check, how this change influences I observer change in behavior.
My prepared SBT version (based on 0.13 branch) used
but from different context,and what it most important, this error caused Why don't you rethrow the error and catch it inside Maven code? Something like this: static
{
try
{
out = new PrintStream( wrapOutputStream( system_out ) );
err = new PrintStream( wrapOutputStream( system_err, STDERR_FILENO ) );
}
catch ( final UnsatisfiedLinkError e )
{
// Failure loading native library.
out = system_out;
err = system_err;
throw e;
}
} |
I don't think it makes sense to error out while loading the AnsiConsole class if it can handle the case where the natives can't be loaded. I do think the PR #54 is doing the right thing. Actually I think the issue is in wrapOutputStream.. let me take a peek at it later today. |
I just use both Maven and SBT (even wrote a Maven plugin to call SBT like AntRun plugin calls Ant, so I had to learn about SBT bootstraping). Now I saw this issue and think that your fix may break SBT usage of Jansi. Just that. It's up to you, what you decide. |
About showing an error, imagine users asking "why I don't see colors?" If you rethrow the exception, Maven can catch it on its side and print debug nice debug message about the problem. There are many benefits of that solution:
|
…on, fallback to striping Ansi codes.
Ok I've just committed a change that should handle this better. @gslowikowski could you re-check against a 1.14-SNAPSHOT build? |
Just catching |
made another small change, in the unix case we should just default to passing the ansi codes through we if can't load the native to tell if the user is on a tty. |
I've tested and found something I didn't see before. The first problem (difference in behaviour between Previously this block of code caught the error and initialized fallback implementation: try {
return new WindowsAnsiOutputStream(stream);
} catch (Throwable ignore) {
// this happens when JNA is not in the path.. or
// this happens when the stdout is being redirected to a file.
} Since public static OutputStream wrapOutputStream(final OutputStream stream) {
return wrapOutputStream(stream, STDOUT_FILENO);
} because reference to Side note: private static boolean detectAnsiSupport() {
OutputStream out = AnsiConsole.wrapOutputStream(new ByteArrayOutputStream());
try {
out.close();
}
catch (Exception e) {
// ignore;
}
return out instanceof WindowsAnsiOutputStream;
} This method is only for Windows. It works only if |
Cannot reproduce this now for quite some time. Tried on HP-UX recently. |
We, the Apache Maven team, are currently preparing Maven 3.4 with color logging support. Running a snapshot from Maven master on a platform which is not supported (no
so
available), drowns the entire VM.See example for FreeBSD:
This doesn't really help because we do not know our users up front and can't have it fail. Jansi shall fall back in no-color mode and emit an error on
stderr
. With this, it is guaranteed that Maven continues to work without color support. See reference thread. Issues MNG-3507 and MNG-6038.I already have a fork of fusesource/hawtjni which makes it perfectly compile on FreeBSD 9 and 10 (tests) pending and local snapshots of all components with colored logging on FreeBSD.
FreeBSD is just an example, this will also happen on Solaris, AIX, etc. I assume that
isatty()
does not fail gracefully?@hboutemy @jdillon
The text was updated successfully, but these errors were encountered: