SOLR-7962: Passing additional arguments to solr.cmd using "--jvm-opts" (formerly "-a") does not work with "-e" on Windows (#3347)

* --jvm-opts (formerly '-a') option doesn't kick in on Windows
* fixing regression in --jvm-opts on linux
* add bats test for --jvm-opts
* platform dependent logging for start script

Author: rahulgoswami

solr/CHANGES.txt

@@ -293,6 +293,9 @@ Bug Fixes
 * SOLR-17758: The NumFieldLimitingUpdateRequestProcessor's "warnOnly" mode has been fixed, and now processes documents even
   when the limit has been exceeded. (Jason Gerlowski, Rahul Goswami)
 
+* SOLR-7962: Passing additional arguments to solr.cmd using "--jvm-opts" (formerly "-a") in conjunction with "-e" (examples like 'techproducts') 
+  wouldn't reflect on Windows (Rahul Goswami via Eric Pugh)
+
 Dependency Upgrades
 ---------------------
 * SOLR-17471: Upgrade Lucene to 9.12.1. (Pierre Salagnac, Christine Poerschke)

solr/bin/solr.cmd

@@ -752,7 +752,7 @@ IF NOT "%EXAMPLE%"=="" (
     -Dlog4j.configurationFile="file:///%DEFAULT_SERVER_DIR%\resources\log4j2-console.xml" ^
     -Dsolr.install.symDir="%SOLR_TIP%" ^
     -classpath "%DEFAULT_SERVER_DIR%\solr-webapp\webapp\WEB-INF\lib\*;%DEFAULT_SERVER_DIR%\lib\ext\*" ^
-    org.apache.solr.cli.SolrCLI run_example --script "%SDIR%\solr.cmd" -e %EXAMPLE% --server-dir "%SOLR_SERVER_DIR%" ^
+    org.apache.solr.cli.SolrCLI run_example --script "%SDIR%\solr.cmd" -e %EXAMPLE% --server-dir "%SOLR_SERVER_DIR%" --jvm-opts "%SOLR_ADDL_ARGS%" ^
     --url-scheme !SOLR_URL_SCHEME! !PASS_TO_RUN_EXAMPLE!
 
   REM End of run_example

solr/core/src/java/org/apache/solr/cli/RunExampleTool.java

@@ -230,17 +230,15 @@ public void runImpl(CommandLine cli) throws Exception {
         throw new IllegalArgumentException(
             "Value of --script option is invalid! " + script + " not found");
     } else {
-      Path scriptFile = serverDir.getParent().resolve("bin").resolve("solr");
+      Path scriptFile =
+          serverDir.getParent().resolve("bin").resolve(CLIUtils.isWindows() ? "solr.cmd" : "solr");
       if (Files.isRegularFile(scriptFile)) {
         script = scriptFile.toAbsolutePath().toString();
       } else {
-        scriptFile = serverDir.getParent().resolve("bin").resolve("solr.cmd");
-        if (Files.isRegularFile(scriptFile)) {
-          script = scriptFile.toAbsolutePath().toString();
-        } else {
-          throw new IllegalArgumentException(
-              "Cannot locate the bin/solr script! Please pass --script to this application.");
-        }
+        throw new IllegalArgumentException(
+            "Cannot locate the bin/"
+                + scriptFile.getFileName().toString()
+                + " script! Please pass --script to this application.");
       }
     }
 
@@ -688,7 +686,8 @@ protected Map<String, Object> startSolr(
     String verboseArg = isVerbose() ? "--verbose" : "";
 
     String jvmOpts = cli.getOptionValue(JVM_OPTS_OPTION);
-    String jvmOptsArg = (jvmOpts != null) ? " --jvm-opts \"" + jvmOpts + "\"" : "";
+    String jvmOptsArg =
+        (jvmOpts != null && !jvmOpts.isEmpty()) ? " --jvm-opts \"" + jvmOpts + "\"" : "";
 
     Path cwd = Path.of(System.getProperty("user.dir"));
     Path binDir = Path.of(script).getParent();
@@ -708,10 +707,10 @@ protected Map<String, Object> startSolr(
             ? "-Dsolr.modules=clustering,extraction,langid,ltr,scripting -Dsolr.ltr.enabled=true -Dsolr.clustering.enabled=true"
             : "";
 
-    String startCmd =
+    String startCmdStr =
         String.format(
             Locale.ROOT,
-            "\"%s\" start %s -p %d --solr-home \"%s\" --server-dir \"%s\" %s %s %s %s %s %s %s %s",
+            "\"%s\" start %s -p %d --solr-home \"%s\" --server-dir \"%s\" %s %s %s %s %s %s %s",
             callScript,
             cloudModeArg,
             port,
@@ -723,12 +722,11 @@ protected Map<String, Object> startSolr(
             forceArg,
             verboseArg,
             extraArgs,
-            jvmOptsArg,
             syspropArg);
-    startCmd = startCmd.replaceAll("\\s+", " ").trim(); // for pretty printing
+    startCmdStr = startCmdStr.replaceAll("\\s+", " ").trim(); // for pretty printing
 
     echo("\nStarting up Solr on port " + port + " using command:");
-    echo(startCmd + "\n");
+    echo(startCmdStr + jvmOptsArg + "\n");
 
     String solrUrl =
         String.format(
@@ -757,8 +755,21 @@ protected Map<String, Object> startSolr(
         }
       }
       DefaultExecuteResultHandler handler = new DefaultExecuteResultHandler();
-      executor.execute(org.apache.commons.exec.CommandLine.parse(startCmd), startEnv, handler);
-
+      org.apache.commons.exec.CommandLine startCmd =
+          org.apache.commons.exec.CommandLine.parse(startCmdStr);
+
+      if (!jvmOptsArg.isEmpty()) {
+        startCmd.addArgument("--jvm-opts");
+
+        /* CommandLine.parse() tends to strip off the quotes by default before sending to cmd.exe.
+        This may cause cmd to break up the argument value on certain characters in unintended ways
+        thereby passing incorrect value to start.cmd
+        (eg: for "-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:18983", it
+        breaks apart the value at "-agentlib:jdwp" passing incorrect args to start.cmd ).
+        The 'false' here tells Exec: “don’t touch my quotes—this is one atomic argument” */
+        startCmd.addArgument("\"" + jvmOpts + "\"", false);
+      }
+      executor.execute(startCmd, startEnv, handler);
       // wait for execution.
       try {
         handler.waitFor(3000);
@@ -767,21 +778,25 @@ protected Map<String, Object> startSolr(
         Thread.interrupted();
       }
       if (handler.hasResult() && handler.getExitValue() != 0) {
+        startCmdStr += jvmOptsArg;
         throw new Exception(
             "Failed to start Solr using command: "
-                + startCmd
+                + startCmdStr
                 + " Exception : "
                 + handler.getException());
       }
     } else {
+      // Unlike Windows, special handling of jvmOpts is not required on linux. We can simply
+      // concatenate to form the complete command
+      startCmdStr += jvmOptsArg;
       try {
-        code = executor.execute(org.apache.commons.exec.CommandLine.parse(startCmd));
+        code = executor.execute(org.apache.commons.exec.CommandLine.parse(startCmdStr));
       } catch (ExecuteException e) {
         throw new Exception(
-            "Failed to start Solr using command: " + startCmd + " Exception : " + e);
+            "Failed to start Solr using command: " + startCmdStr + " Exception : " + e);
       }
+      if (code != 0) throw new Exception("Failed to start Solr using command: " + startCmdStr);
     }
-    if (code != 0) throw new Exception("Failed to start Solr using command: " + startCmd);
 
     return getNodeStatus(
         solrUrl, cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION), maxWaitSecs);

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

@@ -124,7 +124,7 @@ public static CommandLine parseCmdLine(Tool tool, String[] args) {
     List<String> dashDList = new ArrayList<>();
     for (int a = 1; a < args.length; a++) {
       String arg = args[a];
-      if (arg.startsWith("-D")) {
+      if (!args[a - 1].equals("--jvm-opts") && arg.startsWith("-D")) {
         dashDList.add(arg);
       } else {
         toolArgList.add(arg);

solr/packaging/test/test_example.bats

@@ -0,0 +1,41 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+  common_clean_setup
+}
+
+teardown() {
+  # save a snapshot of SOLR_HOME for failed tests
+  save_home_on_failure
+
+  solr stop --all >/dev/null 2>&1
+}
+
+@test "start -e cloud works with --jvm-opts" {
+  solr start -e cloud --no-prompt --jvm-opts "-Dcustom.prop=helloworld"
+  solr assert --started http://localhost:${SOLR_PORT} --cloud http://localhost:${SOLR_PORT} --timeout 60000
+  solr assert --started http://localhost:${SOLR2_PORT} --cloud http://localhost:${SOLR2_PORT} --timeout 60000
+
+  run curl "http://localhost:${SOLR_PORT}/solr/admin/info/properties"
+  assert_output --partial 'helloworld'
+  
+  run curl "http://localhost:${SOLR2_PORT}/solr/admin/info/properties"
+  assert_output --partial 'helloworld'
+}