diff --git a/README.md b/README.md index ff668d5b..2d40f7c9 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,7 @@ $END ``` > [!NOTE] -> While command line processing can exploit all command line functionality (e.g., use environmental variables and wildcards in pathnames), this cannot be done in parameters' files. +> While command line processing can exploit all command line functionality (e.g., use environmental variables and wildcards in pathnames), this cannot be done in parameters' files. The exception are Shell jobs (`APP: SHELL`) that are run via a system shell. In such jobs wildcards and related shell features in `CMD` / `ARGS` are expanded by the shell. Other parameter values are still taken literally (they are not passed through a shell). To use a parameters' file, call AutoCompChem and give it the pathname to the parameters' file as value of the `-p` (`--params`) argument: ``` diff --git a/src/main/java/autocompchem/run/ShellJob.java b/src/main/java/autocompchem/run/ShellJob.java index 9009b8af..ae117083 100644 --- a/src/main/java/autocompchem/run/ShellJob.java +++ b/src/main/java/autocompchem/run/ShellJob.java @@ -22,8 +22,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import java.util.regex.Matcher; -import java.util.regex.Pattern; +import java.util.stream.Collectors; import com.google.gson.JsonElement; import com.google.gson.JsonObject; @@ -31,13 +30,19 @@ import com.google.gson.JsonSerializer; import autocompchem.datacollections.NamedData; -import autocompchem.utils.StringUtils; import autocompchem.utils.TimeUtils; /** * A shell job is work to be done by the shell. The shell command can be executed * in a newly created subfolder. In this case any pathname should reflect the * fact that `pwd` would return the pathname of the subfolder. + *

+ * Commands are always launched via a system shell ({@code sh -c} or + * {@code cmd /c}) so that the shell can expand wildcards and other shell + * features in {@code CMD} / {@code ARGS} text. The {@code CMD} and + * {@code EXE}+{@code SCRIPT} parameter styles remain available; they only + * differ in how the command line string is assembled before it is handed to + * the shell. * * @author Marco Foscato */ @@ -45,7 +50,8 @@ public class ShellJob extends Job { /** - * The command will try to run + * The logical command components (for serialization / debugging). + * Execution always goes through a shell; see {@link #runThisJobSubClassSpecific()}. */ private List command; @@ -127,7 +133,8 @@ public Job makeInstance() //------------------------------------------------------------------------------ /** - * Runs this SHELL command + * Runs this SHELL command via a system shell so that wildcards and related + * shell features in the command line are expanded by the interpreter. */ @Override @@ -142,13 +149,14 @@ public void runThisJobSubClassSpecific() + "in a shell job. Use either one or the other."); } + String commandLine = ""; + // First we need to see if the command comes from the constructor or // from parameter storage if (params.contains(ShellJobConstants.LABINTERPRETER)) { - command = new ArrayList(); - command.add(params.getParameter( - ShellJobConstants.LABINTERPRETER).getValueAsString()); + String interpreter = params.getParameter( + ShellJobConstants.LABINTERPRETER).getValueAsString(); if (!params.contains(ShellJobConstants.LABSCRIPT)) { @@ -161,43 +169,58 @@ public void runThisJobSubClassSpecific() ShellJobConstants.LABSCRIPT).getValueAsString(); script = script.replaceFirst("^~", System.getProperty("user.home")); File scriptFile = getNewFile(script); - command.add(scriptFile.getAbsolutePath()); + String scriptPath = scriptFile.getAbsolutePath(); + + command = new ArrayList(); + command.add(interpreter); + command.add(scriptPath); + // Quote EXE/SCRIPT so paths with spaces stay one word; leave + // ARGS raw so the shell can parse quotes and expand globs. + commandLine = shellQuote(interpreter) + " " + shellQuote(scriptPath); } else if (params.contains(ShellJobConstants.LABCOMMAND)) { + String cmd = params.getParameter( + ShellJobConstants.LABCOMMAND).getValueAsString(); command = new ArrayList(); - Pattern regexMatchingArgs = Pattern.compile( - "[^\\s\"']+|\"[^\"]*\"|'[^']*'"); - Matcher matcher = regexMatchingArgs.matcher(params.getParameter( - ShellJobConstants.LABCOMMAND).getValueAsString()); - while (matcher.find()) - { - command.add(matcher.group()); - } + command.add(cmd); + // Raw CMD text: shell tokenizes and expands wildcards. + commandLine = cmd; + } else if (command != null && !command.isEmpty()) + { + // Constructor-built components: quote each token so a single + // multi-word args component stays one argv entry (historical + // ProcessBuilder behaviour), while unquoted-safe globs still expand. + commandLine = command.stream() + .map(ShellJob::shellQuote) + .collect(Collectors.joining(" ")); } if (params.contains(ShellJobConstants.LABARGS)) { - Pattern regexMatchingArgs = Pattern.compile( - "[^\\s\"']+|\"[^\"]*\"|'[^']*'"); - Matcher matcher = regexMatchingArgs.matcher(params.getParameter( - ShellJobConstants.LABARGS).getValueAsString()); - while (matcher.find()) + String args = params.getParameter( + ShellJobConstants.LABARGS).getValueAsString(); + if (command == null) + { + command = new ArrayList(); + } + command.add(args); + if (!commandLine.isEmpty()) { - command.add(matcher.group()); + commandLine += " "; } + // Raw ARGS: shell parses quotes and expands globs. + commandLine += args; } logger.info("Running " + appID + " Job: " + this.toString() + " Thread: " + Thread.currentThread().getName() + " " + TimeUtils.getTimestamp()); - - String commandAsString = StringUtils.mergeListToString(command, " "); - if (!commandAsString.trim().isEmpty()) + if (commandLine != null && !commandLine.trim().isEmpty()) { try { - ProcessBuilder pb = new ProcessBuilder(command); + ProcessBuilder pb = new ProcessBuilder(wrapInShell(commandLine)); if (customUserDir != null) { // Here is where we move to the work space @@ -249,13 +272,60 @@ public void runThisJobSubClassSpecific() catch (Throwable t) { throw new RuntimeException("Error while running command line " - + "operation '" + commandAsString + "'.", t); + + "operation '" + commandLine + "'.", t); } } logger.info("Done with " + appID + " Job " + this.toString() + " " + TimeUtils.getTimestamp()); } + +//------------------------------------------------------------------------------ + + /** + * Wraps a command line so it is interpreted by a system shell. + * @param commandLine the full command line to run. + * @return argv for {@link ProcessBuilder}: shell, flag, command line. + */ + static List wrapInShell(String commandLine) + { + if (isWindows()) + { + return Arrays.asList("cmd.exe", "/c", commandLine); + } + return Arrays.asList("/bin/sh", "-c", commandLine); + } + +//------------------------------------------------------------------------------ + + /** + * Quotes {@code s} for inclusion in a POSIX {@code sh -c} command line. + * Strings that contain only path-safe characters and glob metacharacters + * ({@code * ? [ ]}) are left unquoted so the shell can expand them. + * @param s the token to quote. + * @return a shell-safe token. + */ + static String shellQuote(String s) + { + if (s == null || s.isEmpty()) + { + return "''"; + } + // Unquoted: avoid spaces and shell metacharacters that are not globs + if (s.matches("[A-Za-z0-9_./:@%+=,\\-\\*\\?\\[\\]]+")) + { + return s; + } + return "'" + s.replace("'", "'\\''") + "'"; + } + +//------------------------------------------------------------------------------ + + private static boolean isWindows() + { + String os = System.getProperty("os.name"); + return os != null && os.toLowerCase().contains("win"); + } //------------------------------------------------------------------------------ diff --git a/src/test/java/autocompchem/run/ShellJobTest.java b/src/test/java/autocompchem/run/ShellJobTest.java index 0959c218..879bab69 100644 --- a/src/test/java/autocompchem/run/ShellJobTest.java +++ b/src/test/java/autocompchem/run/ShellJobTest.java @@ -25,6 +25,7 @@ import java.io.File; import java.io.FileWriter; +import java.nio.file.Files; import java.util.Arrays; import java.util.HashSet; import java.util.List; @@ -33,6 +34,7 @@ import org.junit.jupiter.api.condition.DisabledOnOs; import org.junit.jupiter.api.io.TempDir; +import autocompchem.datacollections.ParameterStorage; import autocompchem.files.FileAnalyzer; @@ -107,6 +109,79 @@ public void testExposeShellOutputEnv() throws Exception } } +//------------------------------------------------------------------------------ + + @Test + @DisabledOnOs(WINDOWS) + public void testShellExpandsWildcardsInCmd() throws Exception + { + assertTrue(this.tempDir.isDirectory(), "Should be a directory"); + + Files.writeString(new File(tempDir, "alpha.txt").toPath(), "A"); + Files.writeString(new File(tempDir, "beta.txt").toPath(), "B"); + Files.writeString(new File(tempDir, "other.dat").toPath(), "X"); + + ParameterStorage params = new ParameterStorage(); + params.setParameter(ShellJobConstants.LABCOMMAND, + "printf '%s\\n' *.txt"); + + ShellJob job = new ShellJob(); + job.setParameters(params); + job.setUserDirAndStdFiles(tempDir); + job.setRedirectOutErr(true); + job.run(); + + File outFile = (File) job.getOutput("LOG").getValue(); + String log = Files.readString(outFile.toPath()); + assertTrue(log.contains("alpha.txt"), "Glob should match alpha.txt"); + assertTrue(log.contains("beta.txt"), "Glob should match beta.txt"); + assertFalse(log.contains("other.dat"), "Glob should not match other.dat"); + } + +//------------------------------------------------------------------------------ + + @Test + @DisabledOnOs(WINDOWS) + public void testShellExpandsWildcardsInExeArgs() throws Exception + { + assertTrue(this.tempDir.isDirectory(), "Should be a directory"); + + Files.writeString(new File(tempDir, "one.dat").toPath(), "1"); + Files.writeString(new File(tempDir, "two.dat").toPath(), "2"); + + File script = new File(tempDir, "countargs.sh"); + Files.writeString(script.toPath(), + "#!/bin/sh" + NL + "echo \"N=$#\"" + NL + "echo \"ALL=$*\"" + NL); + + ParameterStorage params = new ParameterStorage(); + params.setParameter(ShellJobConstants.LABINTERPRETER, "/bin/sh"); + params.setParameter(ShellJobConstants.LABSCRIPT, script.getAbsolutePath()); + params.setParameter(ShellJobConstants.LABARGS, "*.dat"); + + ShellJob job = new ShellJob(); + job.setParameters(params); + job.setUserDirAndStdFiles(tempDir); + job.setRedirectOutErr(true); + job.run(); + + File outFile = (File) job.getOutput("LOG").getValue(); + String log = Files.readString(outFile.toPath()); + assertTrue(log.contains("N=2"), "Wildcard should expand to two args"); + assertTrue(log.contains("one.dat"), "Should receive one.dat"); + assertTrue(log.contains("two.dat"), "Should receive two.dat"); + } + +//------------------------------------------------------------------------------ + + @Test + public void testShellQuoteLeavesGlobsUnquoted() + { + assertEquals("*.txt", ShellJob.shellQuote("*.txt")); + assertEquals("file?.log", ShellJob.shellQuote("file?.log")); + assertEquals("'/path/with space'", ShellJob.shellQuote("/path/with space")); + assertEquals("''", ShellJob.shellQuote("")); + } + //------------------------------------------------------------------------------ } diff --git a/test/cli31.check b/test/cli31.check index e776798e..79a6c539 100755 --- a/test/cli31.check +++ b/test/cli31.check @@ -7,10 +7,10 @@ function not_passed() { if [ ! -f "cli31.log" ] ; then not_passed $LINENO ; fi -if ! grep -q -i " varB=-2.5 " "cli31.log" ; then not_passed $LINENO ; fi -if ! grep -q -i " varB=-0.5 " "cli31.log" ; then not_passed $LINENO ; fi -if ! grep -q -i " varB=1.5 " "cli31.log" ; then not_passed $LINENO ; fi -if ! grep -q -i " varB=19.5 " "cli31.log" ; then not_passed $LINENO ; fi +if ! grep -q -i " varB=-2.5$" "cli31.log" ; then not_passed $LINENO ; fi +if ! grep -q -i " varB=-0.5$" "cli31.log" ; then not_passed $LINENO ; fi +if ! grep -q -i " varB=1.5$" "cli31.log" ; then not_passed $LINENO ; fi +if ! grep -q -i " varB=19.5$" "cli31.log" ; then not_passed $LINENO ; fi if ! grep -q -i " varA=0.0 " "cli31.log" ; then not_passed $LINENO ; fi if ! grep -q -i " varA=13.53 " "cli31.log" ; then not_passed $LINENO ; fi diff --git a/test/cli31.json b/test/cli31.json index f1f75f7f..b2429369 100644 --- a/test/cli31.json +++ b/test/cli31.json @@ -22,14 +22,14 @@ }, { "reference": "CMD", - "value": "echo Iteration=ACCITERNUMBER varA=ACCVALEVARA varB=ACCVALEVARB ; exit -ACCITERNUMBER" + "value": "echo Iteration=ACCITERNUMBER varA=ACCVALEVARA varB=ACCVALEVARB ; exit -ACCITERINTNUMBER" } ] } }, { "reference": "replacementRules", - "value": "ACCITERNUMBER ${x}\nACCVALEVARA ${x*1.23}\nACCVALEVARB ${2*x - 2.5}" + "value": "ACCITERNUMBER ${x}\nACCITERINTNUMBER ${format('0', x)}\nACCVALEVARA ${x*1.23}\nACCVALEVARB ${2*x - 2.5}" }, { "reference": "maxIterations", @@ -40,4 +40,4 @@ "value": "4" } ] -} \ No newline at end of file +} diff --git a/test/cli32.check b/test/cli32.check index b9795ce0..f408baf1 100755 --- a/test/cli32.check +++ b/test/cli32.check @@ -11,7 +11,7 @@ if ! grep -q -i "Iteration=11\.0 " "cli32.log" ; then not_passed $LINENO ; fi if ! grep -q -i " varA=3\.87933" "cli32.log" ; then not_passed $LINENO ; fi if ! grep -q -i " varA=14\.8793" "cli32.log" ; then not_passed $LINENO ; fi -n=0; n=$(grep -c -i " varB=\[4\.070.*, 4\.070.*\] " "cli32.log") +n=0; n=$(grep -c -i " varB=\[4\.070.*, 4\.070.*\]$" "cli32.log") if [ 12 != "$n" ] ; then not_passed $LINENO ; exit 0 ; fi n=0; n=$(grep -c -i "Initiating Shell Job" "cli32.log") diff --git a/test/cli32.json b/test/cli32.json index de32e339..e455bf65 100644 --- a/test/cli32.json +++ b/test/cli32.json @@ -52,14 +52,14 @@ }, { "reference": "CMD", - "value": "echo Iteration=ACCITERNUMBER varA=ACCVALEVARA varB=ACCVALEVARB ; exit -ACCITERNUMBER" + "value": "echo Iteration=ACCITERNUMBER varA=ACCVALEVARA varB=ACCVALEVARB ; exit -ACCITERINTNUMBER" } ] } }, { "reference": "replacementRules", - "value": "ACCITERNUMBER ${x}\nACCVALEVARA ${x + getACCJobsData(#-1.0,mol-0_RuRuDist,0)}\nACCVALEVARB getACCJobsData(#-1.0,mol-0_CCDist)" + "value": "ACCITERNUMBER ${x}\nACCITERINTNUMBER ${format('0', x)}\nACCVALEVARA ${x + getACCJobsData(#-1.0,mol-0_RuRuDist,0)}\nACCVALEVARB getACCJobsData(#-1.0,mol-0_CCDist)" }, { "reference": "maxIterations", diff --git a/test/cli56.check b/test/cli56.check new file mode 100755 index 00000000..88d6ebb8 --- /dev/null +++ b/test/cli56.check @@ -0,0 +1,32 @@ +#!/bin/bash + +function not_passed() { + echo "NOT Passed: check condition leading to line '$1' of '$0'" + exit -1 +} + +if [ ! -f "cli56.log" ] ; then not_passed $LINENO ; fi + +n=0; n=$(find cli56_wdir -maxdepth 1 -name '*.txt' | wc -l | awk '{print $1}') +if [ 0 != "$n" ] ; then not_passed $LINENO ; fi + +n=0; n=$(find cli56_wdir -maxdepth 1 -name '*.dat' | wc -l | awk '{print $1}') +if [ 0 != "$n" ] ; then not_passed $LINENO ; fi + +n=0; n=$(find cli56_wdir -maxdepth 1 -name '*dd' | wc -l | awk '{print $1}') +if [ 1 != "$n" ] ; then not_passed $LINENO ; fi + +n=0; n=$(find cli56_wdir -maxdepth 1 -name 'cli56' | wc -l | awk '{print $1}') +if [ 1 != "$n" ] ; then not_passed $LINENO ; fi + +n=0; n=$(find cli56_wdir -maxdepth 1 -name 'cmd.log' | wc -l | awk '{print $1}') +if [ 1 != "$n" ] ; then not_passed $LINENO ; fi + +n=0; n=$(find cli56_wdir -maxdepth 1 -type f | wc -l | awk '{print $1}') +if [ 3 != "$n" ] ; then not_passed $LINENO ; fi + +if ! grep -q "My text" "cli56_wdir/cmd.log" ; then not_passed $LINENO ; fi + +if grep -q 'Termination status: 0' "cli56.log" ; then echo Passed ; exit 0 ; fi +not_passed $LINENO +exit -1 diff --git a/test/cli56.json b/test/cli56.json new file mode 100644 index 00000000..71cb3517 --- /dev/null +++ b/test/cli56.json @@ -0,0 +1,39 @@ +{ + "jobType": "ACCJob", + "steps": [ + { + "jobType": "ShellJob", + "params": [ + { + "reference": "APP", + "value": "SHELL" + }, + { + "reference": "workDir", + "value": "cli56_wdir" + }, + { + "reference": "CMD", + "value": "touch any_basename.dat cli56 cli56.dat boh.csv cli56.txt other.txt.dd" + } + ] + }, + { + "jobType": "ShellJob", + "params": [ + { + "reference": "APP", + "value": "SHELL" + }, + { + "reference": "workDir", + "value": "cli56_wdir" + }, + { + "reference": "CMD", + "value": "rm -rf *.txt *.dat boh.csv ; echo My text > cmd.log" + } + ] + } + ] +} diff --git a/test/cli56.sh b/test/cli56.sh new file mode 100644 index 00000000..ef76ecaa --- /dev/null +++ b/test/cli56.sh @@ -0,0 +1 @@ +"$javaDir/java" -jar "$ACCHome/target/autocompchem-$accVersion-jar-with-dependencies.jar" -j ../cli56.json > cli56.log diff --git a/test/t127.check b/test/t127.check index dfaa6e73..f225b47d 100755 --- a/test/t127.check +++ b/test/t127.check @@ -13,7 +13,8 @@ fi if ! grep -q "1 is _a_" t127.log ; then echo NOT Passes ERROR 2 ; exit -1 ; fi if ! grep -q "2 is _b_" t127.log ; then echo NOT Passes ERROR 3 ; exit -1 ; fi -if ! grep -q "3 is _\" c c c\"_" t127.log ; then echo NOT Passes ERROR 4 ; exit -1 ; fi +# Shell strips the quotes; the third argv is the spaced string itself +if ! grep -q "3 is _ c c c_" t127.log ; then echo NOT Passes ERROR 4 ; exit -1 ; fi if ! grep -q "Tot is _3_" t127.log ; then echo NOT Passes ERROR 5 ; exit -1 ; fi grep -q 'Termination status: 0' t127.log diff --git a/test/t128.check b/test/t128.check index 9f8b9be6..9154bb5e 100755 --- a/test/t128.check +++ b/test/t128.check @@ -13,7 +13,8 @@ fi if ! grep -q "1 is _a_" t128.log ; then echo NOT Passes ERROR 2 ; exit -1 ; fi if ! grep -q "2 is _b_" t128.log ; then echo NOT Passes ERROR 3 ; exit -1 ; fi -if ! grep -q "3 is _\" c c c\"_" t128.log ; then echo NOT Passes ERROR 4 ; exit -1 ; fi +# Shell strips the quotes; the third argv is the spaced string itself +if ! grep -q "3 is _ c c c_" t128.log ; then echo NOT Passes ERROR 4 ; exit -1 ; fi if ! grep -q "Tot is _3_" t128.log ; then echo NOT Passes ERROR 5 ; exit -1 ; fi grep -q 'Termination status: 0' t128.log