From 0b05425aa5bd7999c59533004349fdd35801ef18 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 25 Mar 2022 12:25:30 -0500 Subject: [PATCH] Issue #5681 - clearer warning on JVM Arg / System Property use (#7769) - Improved warning on unrecognized Command Line Options Signed-off-by: Joakim Erdfelt --- .../java/org/eclipse/jetty/start/Main.java | 6 +- .../org/eclipse/jetty/start/StartArgs.java | 59 +++++++++++++------ .../org/eclipse/jetty/start/MainTest.java | 51 ++++++++++++++++ .../org/eclipse/jetty/start/PropertyDump.java | 2 - .../jetty/start/usecases/BasicTest.java | 2 +- 5 files changed, 98 insertions(+), 22 deletions(-) diff --git a/jetty-core/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java b/jetty-core/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java index 97fa87359a3b..3709b52a402c 100644 --- a/jetty-core/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java +++ b/jetty-core/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java @@ -503,7 +503,11 @@ else if (args.isCreateFiles() || !args.getStartModules().isEmpty()) if (args.hasJvmArgs() || args.hasSystemProperties()) { - StartLog.warn("System properties and/or JVM args set. Consider using --dry-run or --exec"); + StartLog.warn("Unknown Arguments detected. Consider using --dry-run or --exec"); + if (args.hasSystemProperties()) + args.getSystemProperties().forEach((k, v) -> StartLog.warn(" Argument: -D%s=%s (interpreted as a System property, from %s)", k, System.getProperty(k), v)); + if (args.hasJvmArgs()) + args.getJvmArgSources().forEach((jvmArg, source) -> StartLog.warn(" Argument: %s (interpreted as a JVM argument, from %s)", jvmArg, source)); } ClassLoader cl = classpath.getClassLoader(); diff --git a/jetty-core/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java b/jetty-core/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java index 1b50c832e17a..89a1427396f7 100644 --- a/jetty-core/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java +++ b/jetty-core/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java @@ -128,7 +128,7 @@ public class StartArgs /** * JVM arguments, found via command line and in all active [exec] sections from enabled modules */ - private final List jvmArgs = new ArrayList<>(); + private final Map jvmArgSources = new LinkedHashMap<>(); private final Map systemPropertySource = new HashMap<>(); @@ -308,21 +308,22 @@ public void dumpJavaEnvironment(PrintStream out) public void dumpJvmArgs(PrintStream out) { - if (jvmArgs.isEmpty()) + if (jvmArgSources.isEmpty()) return; out.println(); out.println("Forked JVM Arguments:"); out.println("---------------------"); - for (String jvmArgKey : jvmArgs) + jvmArgSources.forEach((key, sourceRef) -> { - String value = System.getProperty(jvmArgKey); + String value = System.getProperty(key); + String source = StartLog.isDebugEnabled() ? '(' + sourceRef + ')' : ""; if (value != null) - out.printf(" %s = %s%n", jvmArgKey, value); + out.printf(" %s = %s %s%n", key, value, source); else - out.printf(" %s%n", jvmArgKey); - } + out.printf(" %s %s%n", key, source); + }); } public void dumpSystemProperties(PrintStream out) @@ -430,7 +431,7 @@ public void expandModules(List activeModules) throws IOException for (String jvmArg : module.getJvmArgs()) { exec = true; - jvmArgs.add(jvmArg); + jvmArgSources.put(jvmArg, String.format("module[%s|jvm]", module.getName())); } // Find and Expand XML files @@ -484,9 +485,25 @@ public List getFiles() return files; } + /** + * Gets the List of JVM arguments detected. + * + * @deprecated use {@link #getJvmArgSources()} instead, as it will return source references with each arg. + */ + @Deprecated public List getJvmArgs() { - return jvmArgs; + return new ArrayList<>(jvmArgSources.keySet()); + } + + /** + * Return ordered Map of JVM arguments to Source (locations) + * + * @return the ordered map of JVM Argument to Source (locations) + */ + public Map getJvmArgSources() + { + return jvmArgSources; } public CommandLineBuilder getMainArgs(Set parts) throws IOException @@ -511,7 +528,7 @@ public CommandLineBuilder getMainArgs(Set parts) throws IOException cmd.addRawArg("-Djetty.base=" + baseHome.getBase()); Props properties = coreEnvironment.getProperties(); - for (String x : getJvmArgs()) + for (String x : getJvmArgSources().keySet()) { if (x.startsWith("-D")) { @@ -774,7 +791,10 @@ private void generateJpmsArgs(CommandLineBuilder cmd) public String getMainClassname() { String mainClass = System.getProperty("jetty.server", isJPMS() ? MODULE_MAIN_CLASS : MAIN_CLASS); - return System.getProperty("main.class", mainClass); + Prop mainClassProp = getCoreEnvironment().getProperties().getProp("main.class", true); + if (mainClassProp != null) + return mainClassProp.value; + return mainClass; } public String getMavenLocalRepoDir() @@ -843,7 +863,7 @@ public Set getSources(String module) public boolean hasJvmArgs() { - return !jvmArgs.isEmpty(); + return !jvmArgSources.isEmpty(); } public boolean hasSystemProperties() @@ -861,6 +881,11 @@ public boolean hasSystemProperties() return false; } + public Map getSystemProperties() + { + return systemPropertySource; + } + public boolean isApproveAllLicenses() { return approveAllLicenses; @@ -1274,11 +1299,9 @@ public Environment parse(Environment environment, String arg, String source) // Anything else with a "-" is considered a JVM argument if (arg.startsWith("-")) { - // Only add non-duplicates - if (!jvmArgs.contains(arg)) - { - jvmArgs.add(arg); - } + StartLog.debug("Unrecognized Arg (possible JVM Arg): %s (from %s)", arg, source); + // always use the latest source (overriding any past tracked source) + jvmArgSources.put(arg, source); return environment; } @@ -1464,6 +1487,6 @@ public void setRun(boolean run) public String toString() { return String.format("%s[enabledModules=%s, xml=%s, properties=%s, jvmArgs=%s]", - getClass().getSimpleName(), modules, getCoreEnvironment().getXmlFiles(), getCoreEnvironment().getProperties(), jvmArgs); + getClass().getSimpleName(), modules, getCoreEnvironment().getXmlFiles(), getCoreEnvironment().getProperties(), jvmArgSources.keySet()); } } diff --git a/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java b/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java index f70128dbfe9c..698dcbe43145 100644 --- a/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java +++ b/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java @@ -17,9 +17,12 @@ import java.io.File; import java.io.PrintStream; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Iterator; import java.util.List; +import java.util.stream.Collectors; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.junit.jupiter.api.BeforeEach; @@ -91,6 +94,54 @@ public void testListConfig() throws Exception assertThat("user.dir should indicate that it was specified on the command line", userDirLine, containsString("()")); } + @Test + public void testUnknownDistroCommand() throws Exception + { + List cmdLineArgs = new ArrayList<>(); + File testJettyHome = MavenTestingUtils.getTestResourceDir("dist-home"); + Path testJettyBase = MavenTestingUtils.getTargetTestingPath("base-example-unknown"); + FS.ensureDirectoryExists(testJettyBase); + Path zedIni = testJettyBase.resolve("start.d/zed.ini"); + FS.ensureDirectoryExists(zedIni.getParent()); + Files.writeString(zedIni, "--zed-0-zed"); + cmdLineArgs.add("jetty.home=" + testJettyHome); + cmdLineArgs.add("jetty.base=" + testJettyBase); + cmdLineArgs.add("main.class=" + PropertyDump.class.getName()); + cmdLineArgs.add("--module=base"); + cmdLineArgs.add("--foople"); + cmdLineArgs.add("-Dzed.key=0.value"); + + List output; + + try (ByteArrayOutputStream baos = new ByteArrayOutputStream(); + PrintStream out = new PrintStream(baos, true, StandardCharsets.UTF_8)) + { + PrintStream originalStream = StartLog.setStream(new PrintStream(out)); + try + { + Main main = new Main(); + StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[0])); + main.start(args); + out.flush(); + output = List.of(baos.toString(StandardCharsets.UTF_8).split(System.lineSeparator())); + } + finally + { + StartLog.setStream(originalStream); + } + } + + // Test a System Property that comes from JVM + List warnings = output.stream().filter((line) -> line.startsWith("WARN")).collect(Collectors.toList()); + // warnings.forEach(System.out::println); + Iterator warningIter = warnings.iterator(); + + assertThat("Announcement", warningIter.next(), containsString("Unknown Arguments detected.")); + assertThat("System Prop on command line detail", warningIter.next(), containsString("Argument: -Dzed.key=0.value (interpreted as a System property, from ")); + assertThat("JVM Arg in ini detail", warningIter.next(), containsString("Argument: --zed-0-zed (interpreted as a JVM argument, from " + zedIni)); + assertThat("JVM Arg on command line detail", warningIter.next(), containsString("Argument: --foople (interpreted as a JVM argument, from ")); + } + @Test @Disabled("Just a bit noisy for general testing") public void testHelp() throws Exception diff --git a/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/PropertyDump.java b/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/PropertyDump.java index fee9ebef552a..edb5c73c446f 100644 --- a/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/PropertyDump.java +++ b/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/PropertyDump.java @@ -63,7 +63,5 @@ public static void main(String[] args) } } } - - System.exit(0); } } diff --git a/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java b/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java index 4f101bd731c0..70cd3a753af1 100644 --- a/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java +++ b/jetty-core/jetty-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java @@ -312,7 +312,7 @@ public void testWithCommandLine() throws Exception "-Xms1g", "-Xmx1g" ); - List actualJvmArgs = results.startArgs.getJvmArgs(); + List actualJvmArgs = new ArrayList<>(results.startArgs.getJvmArgSources().keySet()); assertThat("JVM Args", actualJvmArgs, contains(expectedJvmArgs.toArray())); }