Skip to content
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

Fixed isJansiConsoleInstalled performance issue #1976

Merged
merged 8 commits into from
Mar 24, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,11 @@ public void testAnsiAutoJansiConsoleInstalledOverridesHintDisabled() {
environmentVariables.clear(ANSI_ENVIRONMENT_VARIABLES);
environmentVariables.set("CLICOLOR", "0"); // hint disabled
System.setProperty("os.name", "Windows");
// Clear the globally cached jansiConsole value that might
// have been set in a previous test to force the
// Ansi#isJansiConsoleInstalled method to recalculate
// the cached value.
Ansi.jansiConsole = null;
assertTrue(Ansi.isWindows());
assertFalse(Ansi.isPseudoTTY());
assertFalse(Ansi.forceDisabled());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package picocli;


import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.DisabledForJreRange;
import org.junit.jupiter.api.condition.JRE;
import picocli.CommandLine.Help.Ansi;

import java.io.File;
import java.io.FileInputStream;
Expand Down Expand Up @@ -508,6 +510,15 @@ private String expectedCompletionScriptForNonDefault() {
CommandLine.VERSION);
}

@BeforeAll
static void disableAnsi() {
// Clear the globally cached jansiConsole value that might
// have been set in a previous test to force the
// Ansi#isJansiConsoleInstalled method to recalculate
// the cached value.
Ansi.jansiConsole = null;
}

@Test
@DisabledForJreRange(min = JRE.JAVA_18, max = JRE.JAVA_21, disabledReason = "UnsupportedOperationException in SystemLambda.withSecurityManager")
public void testAutoCompleteAppHelp() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,12 @@ public void testAnsiAutoJansiConsoleInstalledOverridesHintDisabled() throws Exce
assertFalse(Ansi.hintEnabled());

assertFalse(Ansi.isJansiConsoleInstalled());

// Clear the globally cached jansiConsole value that might
// have been set in a previous test to force the
// Ansi#isJansiConsoleInstalled method to recalculate
// the cached value.
Ansi.jansiConsole = null;
AnsiConsole.systemInstall();
try {
assertTrue(Ansi.isJansiConsoleInstalled());
Expand All @@ -628,6 +634,7 @@ public void testAnsiAutoHintDisabledOverridesHintEnabled() throws Exception {
restoreSystemProperties(() -> {

System.setProperty("os.name", "Windows");
Ansi.jansiConsole = null;
withEnvironmentVariable(ANSI_ENVIRONMENT_VARIABLES[0], null)
.and(ANSI_ENVIRONMENT_VARIABLES[1], null)
.and(ANSI_ENVIRONMENT_VARIABLES[2], null)
Expand Down Expand Up @@ -688,6 +695,7 @@ public void testAnsiAutoDisabledIfNoTty() throws Exception {
restoreSystemProperties(() -> {

System.setProperty("os.name", "Windows");
Ansi.jansiConsole = null;
assertTrue(Ansi.isWindows());
assertFalse(Ansi.isPseudoTTY());
assertFalse(Ansi.isJansiConsoleInstalled());
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/picocli/CommandLine.java
Original file line number Diff line number Diff line change
Expand Up @@ -17805,7 +17805,14 @@ static boolean ansiPossible() {
if (!isTTY() && !isPseudoTTY()) { return false; }
return hintEnabled() || !isWindows() || isXterm() || isCygwin() || hasOsType();
}
/** Cache the result for isJansiConsoleInstalled so it doesn't repeatedly
* call Class#forName, which can cause performance issues. */
static Boolean jansiConsole;
static boolean isJansiConsoleInstalled() {
if (jansiConsole == null) { jansiConsole = calcIsJansiConsoleInstalled(); }
return jansiConsole;
}
static boolean calcIsJansiConsoleInstalled() {
try {
// first check if JANSI was explicitly disabled _without loading any JANSI classes_:
// see https://github.com/remkop/picocli/issues/1106
Expand Down
4 changes: 4 additions & 0 deletions src/test/java/picocli/HelpAnsiTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@ public void testAnsiEnabled() {

if (isWindows && !Ansi.AUTO.enabled()) {
AnsiConsole.systemInstall();

// The previous Ansi.enabled() call caches the result for whether or not jansi is enabled. Reset the cache value
// and force the Ansi.enabled() call to rescan the classpath for the jansi classes.
Ansi.jansiConsole = null;
try {
assertTrue(Ansi.AUTO.enabled());
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.junit.Test;
import org.junit.contrib.java.lang.system.SystemErrRule;
import picocli.CommandLine.Command;
import picocli.CommandLine.Help.Ansi;
import picocli.CommandLine.IParameterPreprocessor;
import picocli.CommandLine.Model.ArgSpec;
import picocli.CommandLine.Model.CommandSpec;
Expand Down Expand Up @@ -111,6 +112,11 @@ public void testAmbiguousOptionsDefault() {
//-x -y=123
MyCommand obj = new MyCommand();
CommandLine cmdLine = new CommandLine(obj);
// Clear the globally cached jansiConsole value that might
// have been set in a previous test to force the
// Ansi#isJansiConsoleInstalled method to recalculate
// the cached value.
Ansi.jansiConsole = null;
int exitCode = cmdLine.execute("-x", "-y=123");
assertEquals(2, exitCode);
String expected = String.format("" +
Expand Down
5 changes: 4 additions & 1 deletion src/test/java/picocli/ModelTransformerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import org.junit.Test;
import picocli.CommandLine.Command;
import picocli.CommandLine.Help.Ansi;

import java.io.PrintWriter;
import java.io.StringWriter;
Expand Down Expand Up @@ -34,7 +35,9 @@ public CommandLine.Model.CommandSpec transform(CommandLine.Model.CommandSpec com
@Test
public void testUsage() {
StringWriter sw = new StringWriter();
new CommandLine(new MyCommand()).usage(new PrintWriter(sw));
// Explicitly disable Ansi to make sure that the cached isJansiConsoleInstalled
// value doesn't inadvertently cause the usage help to enable ansi.
new CommandLine(new MyCommand()).usage(new PrintWriter(sw), Ansi.OFF);
String expected = String.format("" +
"Usage: mycmd [-hV] [COMMAND]%n" +
" -h, --help Show this help message and exit.%n" +
Expand Down