From 96af5d1990db7c99465890ed4bd3eaf540f620ba 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-start/src/main/java/org/eclipse/jetty/start/Main.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java index 5a0a14a64a79..3de6eabcfe24 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java @@ -501,7 +501,11 @@ public void run() 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-start/src/main/java/org/eclipse/jetty/start/StartArgs.java b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java index 80f125176fbe..b99d46b08ecc 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java @@ -149,7 +149,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<>(); /** * List of all xml references found directly on command line or start.ini @@ -331,21 +331,22 @@ public void dumpEnvironment(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 dumpProperties(PrintStream out) @@ -544,7 +545,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 @@ -655,9 +656,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 @@ -681,7 +698,7 @@ public CommandLineBuilder getMainArgs(Set parts) throws IOException cmd.addRawArg("-Djetty.home=" + baseHome.getHome()); cmd.addRawArg("-Djetty.base=" + baseHome.getBase()); - for (String x : getJvmArgs()) + for (String x : getJvmArgSources().keySet()) { if (x.startsWith("-D")) { @@ -816,7 +833,10 @@ else if (properties.size() > 0) public String getMainClassname() { String mainClass = System.getProperty("jetty.server", isJPMS() ? MODULE_MAIN_CLASS : MAIN_CLASS); - return System.getProperty("main.class", mainClass); + Prop mainClassProp = properties.getProp("main.class", true); + if (mainClassProp != null) + return mainClassProp.value; + return mainClass; } public String getMavenLocalRepoDir() @@ -878,6 +898,11 @@ public Props getProperties() return properties; } + public Map getSystemProperties() + { + return systemPropertySource; + } + public Set getSkipFileValidationModules() { return skipFileValidationModules; @@ -895,7 +920,7 @@ public List getXmlFiles() public boolean hasJvmArgs() { - return !jvmArgs.isEmpty(); + return !jvmArgSources.isEmpty(); } public boolean hasSystemProperties() @@ -1331,11 +1356,9 @@ public void parse(final String rawarg, 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; } @@ -1555,6 +1578,6 @@ public void setRun(boolean run) public String toString() { return String.format("%s[enabledModules=%s, xmlRefs=%s, properties=%s, jvmArgs=%s]", - getClass().getSimpleName(), modules, xmlRefs, properties, jvmArgs); + getClass().getSimpleName(), modules, xmlRefs, properties, jvmArgSources.keySet()); } } diff --git a/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java b/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java index da9247ff468f..04a8c18d72a1 100644 --- a/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java +++ b/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-start/src/test/java/org/eclipse/jetty/start/PropertyDump.java b/jetty-start/src/test/java/org/eclipse/jetty/start/PropertyDump.java index fee9ebef552a..edb5c73c446f 100644 --- a/jetty-start/src/test/java/org/eclipse/jetty/start/PropertyDump.java +++ b/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-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java b/jetty-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java index 0fa4994ed729..e3abadc441a8 100644 --- a/jetty-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java +++ b/jetty-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java @@ -256,7 +256,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())); }