Dependencies: Update to jopt-5.0 (#19278)

The new version of jopt allows us to remove a couple of TODOs in the code.

Closes #12368

core/build.gradle

@@ -59,7 +59,7 @@ dependencies {
   compile 'org.elasticsearch:securesm:1.0'
 
   // utilities
-  compile 'net.sf.jopt-simple:jopt-simple:4.9'
+  compile 'net.sf.jopt-simple:jopt-simple:5.0.2'
   compile 'com.carrotsearch:hppc:0.7.1'
 
   // time handling, remove with java 8 time

core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java

@@ -26,7 +26,6 @@ import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.Version;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.common.PidFile;
-import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.SuppressForbidden;
 import org.elasticsearch.common.inject.CreationException;
 import org.elasticsearch.common.logging.ESLogger;
@@ -180,10 +179,10 @@ final class Bootstrap {
         };
     }
 
-    private static Environment initialSettings(boolean foreground, String pidFile, Map<String, String> esSettings) {
+    private static Environment initialSettings(boolean foreground, Path pidFile, Map<String, String> esSettings) {
         Terminal terminal = foreground ? Terminal.DEFAULT : null;
         Settings.Builder builder = Settings.builder();
-        if (Strings.hasLength(pidFile)) {
+        if (pidFile != null) {
             builder.put(Environment.PIDFILE_SETTING.getKey(), pidFile);
         }
         return InternalSettingsPreparer.prepareEnvironment(builder.build(), terminal, esSettings);
@@ -215,7 +214,7 @@ final class Bootstrap {
      */
     static void init(
             final boolean foreground,
-            final String pidFile,
+            final Path pidFile,
             final Map<String, String> esSettings) throws Exception {
         // Set the system property before anything has a chance to trigger its use
         initLoggerPrefix();

core/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java

@@ -21,6 +21,9 @@ package org.elasticsearch.bootstrap;
 
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
+import joptsimple.OptionSpecBuilder;
+import joptsimple.util.PathConverter;
+import joptsimple.util.PathProperties;
 import org.elasticsearch.Build;
 import org.elasticsearch.cli.ExitCodes;
 import org.elasticsearch.cli.SettingCommand;
@@ -29,6 +32,7 @@ import org.elasticsearch.cli.UserException;
 import org.elasticsearch.monitor.jvm.JvmInfo;
 
 import java.io.IOException;
+import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.Map;
 
@@ -37,22 +41,23 @@ import java.util.Map;
  */
 class Elasticsearch extends SettingCommand {
 
-    private final OptionSpec<Void> versionOption;
-    private final OptionSpec<Void> daemonizeOption;
-    private final OptionSpec<String> pidfileOption;
+    private final OptionSpecBuilder versionOption;
+    private final OptionSpecBuilder daemonizeOption;
+    private final OptionSpec<Path> pidfileOption;
 
     // visible for testing
     Elasticsearch() {
         super("starts elasticsearch");
-        // TODO: in jopt-simple 5.0, make this mutually exclusive with all other options
         versionOption = parser.acceptsAll(Arrays.asList("V", "version"),
             "Prints elasticsearch version information and exits");
         daemonizeOption = parser.acceptsAll(Arrays.asList("d", "daemonize"),
-            "Starts Elasticsearch in the background");
-        // TODO: in jopt-simple 5.0 this option type can be a Path
+            "Starts Elasticsearch in the background")
+            .availableUnless(versionOption);
         pidfileOption = parser.acceptsAll(Arrays.asList("p", "pidfile"),
             "Creates a pid file in the specified path on start")
-            .withRequiredArg();
+            .availableUnless(versionOption)
+            .withRequiredArg()
+            .withValuesConvertedBy(new PathConverter());
     }
 
     /**
@@ -86,12 +91,12 @@ class Elasticsearch extends SettingCommand {
         }
 
         final boolean daemonize = options.has(daemonizeOption);
-        final String pidFile = pidfileOption.value(options);
+        final Path pidFile = pidfileOption.value(options);
 
         init(daemonize, pidFile, settings);
     }
 
-    void init(final boolean daemonize, final String pidFile, final Map<String, String> esSettings) {
+    void init(final boolean daemonize, final Path pidFile, final Map<String, String> esSettings) {
         try {
             Bootstrap.init(!daemonize, pidFile, esSettings);
         } catch (final Throwable t) {

core/src/main/java/org/elasticsearch/cli/Command.java

@@ -41,7 +41,8 @@ public abstract class Command {
 
     private final OptionSpec<Void> helpOption = parser.acceptsAll(Arrays.asList("h", "help"), "show help").forHelp();
     private final OptionSpec<Void> silentOption = parser.acceptsAll(Arrays.asList("s", "silent"), "show minimal output");
-    private final OptionSpec<Void> verboseOption = parser.acceptsAll(Arrays.asList("v", "verbose"), "show verbose output");
+    private final OptionSpec<Void> verboseOption = parser.acceptsAll(Arrays.asList("v", "verbose"), "show verbose output")
+            .availableUnless(silentOption);
 
     public Command(String description) {
         this.description = description;
@@ -77,10 +78,6 @@ public abstract class Command {
         }
 
         if (options.has(silentOption)) {
-            if (options.has(verboseOption)) {
-                // mutually exclusive, we can remove this with jopt-simple 5.0, which natively supports it
-                throw new UserException(ExitCodes.USAGE, "Cannot specify -s and -v together");
-            }
             terminal.setVerbosity(Terminal.Verbosity.SILENT);
         } else if (options.has(verboseOption)) {
             terminal.setVerbosity(Terminal.Verbosity.VERBOSE);

core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

@@ -185,12 +185,7 @@ class InstallPluginCommand extends SettingCommand {
 
     @Override
     protected void execute(Terminal terminal, OptionSet options, Map<String, String> settings) throws Exception {
-        // TODO: in jopt-simple 5.0 we can enforce a min/max number of positional args
-        List<String> args = arguments.values(options);
-        if (args.size() != 1) {
-            throw new UserException(ExitCodes.USAGE, "Must supply a single plugin id argument");
-        }
-        String pluginId = args.get(0);
+        String pluginId = arguments.value(options);
         boolean isBatch = options.has(batchOption) || System.console() == null;
         execute(terminal, pluginId, isBatch, settings);
     }

core/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java

@@ -54,12 +54,8 @@ class RemovePluginCommand extends SettingCommand {
 
     @Override
     protected void execute(Terminal terminal, OptionSet options, Map<String, String> settings) throws Exception {
-        // TODO: in jopt-simple 5.0 we can enforce a min/max number of positional args
-        List<String> args = arguments.values(options);
-        if (args.size() != 1) {
-            throw new UserException(ExitCodes.USAGE, "Must supply a single plugin id argument");
-        }
-        execute(terminal, args.get(0), settings);
+        String arg = arguments.value(options);
+        execute(terminal, arg, settings);
     }
 
     // pkg private for testing

core/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java

@@ -24,10 +24,12 @@ import org.elasticsearch.Version;
 import org.elasticsearch.cli.ExitCodes;
 import org.elasticsearch.monitor.jvm.JvmInfo;
 
+import java.nio.file.Path;
 import java.util.function.Consumer;
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.hasEntry;
 
 public class ElasticsearchCliTests extends ESElasticsearchCliTestCase {
@@ -50,7 +52,8 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase {
                 ExitCodes.USAGE,
                 output -> assertThat(
                         output,
-                        containsString("ERROR: Elasticsearch version option is mutually exclusive with any other option")),
+                        allOf(containsString("ERROR:"),
+                              containsString("are unavailable given other options on the command line"))),
                 args);
     }
 
@@ -91,18 +94,22 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase {
     }
 
     public void testThatPidFileCanBeConfigured() throws Exception {
-        runPidFileTest(ExitCodes.USAGE, false, output -> assertThat(output, containsString("Option p/pidfile requires an argument")), "-p");
-        runPidFileTest(ExitCodes.OK, true, output -> {}, "-p", "/tmp/pid");
-        runPidFileTest(ExitCodes.OK, true, output -> {}, "--pidfile", "/tmp/pid");
+        Path tmpDir = createTempDir();
+        Path pidFile = tmpDir.resolve("pid");
+        runPidFileTest(ExitCodes.USAGE, false,
+                output -> assertThat(output, containsString("Option p/pidfile requires an argument")), pidFile, "-p");
+        runPidFileTest(ExitCodes.OK, true, output -> {}, pidFile, "-p", pidFile.toString());
+        runPidFileTest(ExitCodes.OK, true, output -> {}, pidFile, "--pidfile", tmpDir.toString() + "/pid");
     }
 
-    private void runPidFileTest(final int expectedStatus, final boolean expectedInit, Consumer<String> outputConsumer, final String... args)
+    private void runPidFileTest(final int expectedStatus, final boolean expectedInit, Consumer<String> outputConsumer,
+                                Path expectedPidFile, final String... args)
             throws Exception {
         runTest(
                 expectedStatus,
                 expectedInit,
                 outputConsumer,
-                (foreground, pidFile, esSettings) -> assertThat(pidFile, equalTo("/tmp/pid")),
+                (foreground, pidFile, esSettings) -> assertThat(pidFile.toString(), equalTo(expectedPidFile.toString())),
                 args);
     }
 

core/src/test/java/org/elasticsearch/cli/CommandTests.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.cli;
 
+import joptsimple.OptionException;
 import joptsimple.OptionSet;
 import org.elasticsearch.test.ESTestCase;
 
@@ -87,10 +88,11 @@ public class CommandTests extends ESTestCase {
         MockTerminal terminal = new MockTerminal();
         NoopCommand command = new NoopCommand();
         String[] args = {"-v", "-s"};
-        UserException e = expectThrows(UserException.class, () -> {
+        OptionException e = expectThrows(OptionException.class, () -> {
             command.mainWithoutErrorHandling(args, terminal);
         });
-        assertTrue(e.getMessage(), e.getMessage().contains("Cannot specify -s and -v together"));
+        assertTrue(e.getMessage(),
+                e.getMessage().contains("Option(s) [v/verbose] are unavailable given other options on the command line"));
     }
 
     public void testSilentVerbosity() throws Exception {

distribution/licenses/jopt-simple-4.9.jar.sha1

@@ -1 +0,0 @@
-ee9e9eaa0a35360dcfeac129ff4923215fd65904

distribution/licenses/jopt-simple-5.0.2.jar.sha1

@@ -0,0 +1 @@
+98cafc6081d5632b61be2c9e60650b64ddbc637c

test/framework/src/main/java/org/elasticsearch/bootstrap/ESElasticsearchCliTestCase.java

@@ -22,6 +22,7 @@ package org.elasticsearch.bootstrap;
 import org.elasticsearch.cli.MockTerminal;
 import org.elasticsearch.test.ESTestCase;
 
+import java.nio.file.Path;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Consumer;
@@ -31,7 +32,7 @@ import static org.hamcrest.CoreMatchers.equalTo;
 abstract class ESElasticsearchCliTestCase extends ESTestCase {
 
     interface InitConsumer {
-        void accept(final boolean foreground, final String pidFile, final Map<String, String> esSettings);
+        void accept(final boolean foreground, final Path pidFile, final Map<String, String> esSettings);
     }
 
     void runTest(
@@ -45,7 +46,7 @@ abstract class ESElasticsearchCliTestCase extends ESTestCase {
             final AtomicBoolean init = new AtomicBoolean();
             final int status = Elasticsearch.main(args, new Elasticsearch() {
                 @Override
-                void init(final boolean daemonize, final String pidFile, final Map<String, String> esSettings) {
+                void init(final boolean daemonize, final Path pidFile, final Map<String, String> esSettings) {
                     init.set(true);
                     initConsumer.accept(!daemonize, pidFile, esSettings);
                 }