diff --git a/.idea/compiler.xml b/.idea/compiler.xml deleted file mode 100644 index ebc42bcb..00000000 --- a/.idea/compiler.xml +++ /dev/null @@ -1,6 +0,0 @@ - - - - - \ No newline at end of file diff --git a/.idea/deployment.xml b/.idea/deployment.xml deleted file mode 100644 index 8eb77d14..00000000 --- a/.idea/deployment.xml +++ /dev/null @@ -1,4 +0,0 @@ - - - - \ No newline at end of file diff --git a/.idea/groovyc.xml b/.idea/groovyc.xml deleted file mode 100644 index e82d5941..00000000 --- a/.idea/groovyc.xml +++ /dev/null @@ -1,6 +0,0 @@ - - - - - \ No newline at end of file diff --git a/.idea/libraries/roddyBundledLibraries.xml b/.idea/libraries/roddyBundledLibraries.xml deleted file mode 100644 index bbd8e525..00000000 --- a/.idea/libraries/roddyBundledLibraries.xml +++ /dev/null @@ -1,29 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml deleted file mode 100644 index d659a9b3..00000000 --- a/.idea/misc.xml +++ /dev/null @@ -1,39 +0,0 @@ - - - - - - - - - - - - - - diff --git a/.idea/modules.xml b/.idea/modules.xml deleted file mode 100644 index f63af4e1..00000000 --- a/.idea/modules.xml +++ /dev/null @@ -1,16 +0,0 @@ - - - - - - - - - - - - - - - - \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml deleted file mode 100644 index 8a36c21e..00000000 --- a/.idea/vcs.xml +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - \ No newline at end of file diff --git a/.travis.yml b/.travis.yml index 8a78d6d6..5fbca4ad 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,22 +6,18 @@ cache: directories: - "$HOME/.gradle/caches/" - "$HOME/.gradle/wrapper/" +jdk: + - openjdk8 before_install: -- curl -s get.sdkman.io | bash -- source "$HOME/.sdkman/bin/sdkman-init.sh" -- echo sdkman_auto_answer=true > ~/.sdkman/etc/config -- source "/home/travis/.sdkman/bin/sdkman-init.sh" -- sdk install java 8.0.252-open -- sdk use java 8.0.252-open -- export JAVA_HOME="$HOME/.sdkman/candidates/java/current" -- export JAVA_ROOT="$JAVA_HOME" -- export JRE_HOME="$JAVA_HOME" -- export JDK_HOME="$JAVA_HOME" -- sdk install groovy 2.4.19 -- sdk use groovy 2.4.19 -- groovy --version -- git clone https://github.com/eilslabs/Roddy-Default-Plugin.git dist/plugins/DefaultPlugin -- git clone https://github.com/eilslabs/Roddy-Base-Plugin.git dist/plugins/PluginBase + - curl -s get.sdkman.io | bash + - source "$HOME/.sdkman/bin/sdkman-init.sh" + - echo sdkman_auto_answer=true > ~/.sdkman/etc/config + - source "/home/travis/.sdkman/bin/sdkman-init.sh" + - sdk install groovy 2.4.19 + - sdk use groovy 2.4.19 + - groovy --version + - git clone https://github.com/TheRoddyWMS/Roddy-Default-Plugin.git dist/plugins/DefaultPlugin + - git clone https://github.com/TheRoddyWMS/Roddy-Base-Plugin.git dist/plugins/PluginBase deploy: skip_cleanup: true provider: releases diff --git a/CHANGELIST.md b/CHANGELIST.md index 662bb274..0f5a4f09 100644 --- a/CHANGELIST.md +++ b/CHANGELIST.md @@ -1,5 +1,14 @@ # Changelist +* 3.5.9 + + - The LocalExecutionService ignored errors of asychronously executed commands. + Now errors Roddy detects errors and reports their standard and error output. + - LocalExecutionService always executes commands via `bash -c` (before it + did so only if the process was sychronously executed) + - Update of RoddyToolLib to fix error handling in asynchronous execution and + with multi-threading and command-output processing (StringBuilder->StringBuffer) + * 3.5.8 - Bugfix: Project XML validation didn't exit != 0 in strict mode diff --git a/README.md b/README.md index 7ffc20fd..2e9631ce 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,5 @@ -# What is Roddy? +[![Build Status - Travis](https://travis-ci.org/TheRoddyWMS/Roddy.svg?branch=master)](https://travis-ci.org/TheRoddyWMS/Roddy) +# What is Roddy? Roddy is a framework for development and management of workflows on a batch processing cluster. It has been developed at the German Cancer Research Center (DKFZ) in Heidelberg in the eilslabs group and is used by a number of in-house workflows such as the [PanCancer Alignment Workflow](https://github.com/DKFZ-ODCF/AlignmentAndQCWorkflows) and the [ACEseq workflow](https://github.com/eilslabs/ACEseqWorkflow). The development is now continued in the Omics IT and Data Management Core Facility (ODCF) at the DKFZ. @@ -19,7 +20,7 @@ The following workflows have been developed at the DKFZ based on Roddy as workfl * [ACEseq workflow](https://github.com/DKFZ-ODCF/ACEseqWorkflow) for copy-number variation calling * [InDel-Calling workflow](https://github.com/DKFZ-ODCF/IndelCallingWorkflow) workflow * [Sophia workflow](https://github.com/DKFZ-ODCF/SophiaWorkflow) for structural variation calling - * RNA-seq workflow (to be published) + * [RNA-seq workflow](https://github.com/DKFZ-ODCF/RNAseqWorkflow) * CNVkit for copy-number variation calling on exome data (to be published) * Leaf-Cutter workflow * [Bam-to-FASTQ](https://github.com/TheRoddyWMS/BamToFastqPlugin) plugin diff --git a/Roddy.iml b/Roddy.iml deleted file mode 100644 index e5d17a19..00000000 --- a/Roddy.iml +++ /dev/null @@ -1,81 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - \ No newline at end of file diff --git a/RoddyCore/buildversion.txt b/RoddyCore/buildversion.txt index c70c5711..2aa89138 100644 --- a/RoddyCore/buildversion.txt +++ b/RoddyCore/buildversion.txt @@ -1,2 +1,2 @@ 3.5 -8 +9 diff --git a/RoddyCore/src/de/dkfz/roddy/Constants.java b/RoddyCore/src/de/dkfz/roddy/Constants.java index 6bde4615..f1280fbc 100644 --- a/RoddyCore/src/de/dkfz/roddy/Constants.java +++ b/RoddyCore/src/de/dkfz/roddy/Constants.java @@ -16,8 +16,8 @@ public class Constants { // Application constants ///////////////////////// - public static final String APP_CURRENT_VERSION_STRING = "3.5.8"; - public static final String APP_CURRENT_VERSION_BUILD_DATE = "Thu Jun 25 09:14:30 GMT 2020"; + public static final String APP_CURRENT_VERSION_STRING = "3.5.9"; + public static final String APP_CURRENT_VERSION_BUILD_DATE = "Wed Oct 21 17:08:57 CEST 2020"; public static final String APP_PROPERTY_JOB_MANAGER_CLASS = "jobManagerClass"; public static final String APP_PROPERTY_FILESYSTEM_ACCESS_MANAGER_CLASS = "fileSystemAccessManagerClass"; public static final String APP_PROPERTY_EXECUTION_SERVICE_CLASS = "executionServiceClass"; diff --git a/RoddyCore/src/de/dkfz/roddy/Roddy.java b/RoddyCore/src/de/dkfz/roddy/Roddy.java index 0b48a1ad..4acc6272 100644 --- a/RoddyCore/src/de/dkfz/roddy/Roddy.java +++ b/RoddyCore/src/de/dkfz/roddy/Roddy.java @@ -1032,7 +1032,7 @@ public static ShellCommandSet getLocalCommandSet() { if (RoddyConversionHelperMethods.isNullOrEmpty(localCommandSet)) { logger.postSometimesInfo("Using BashCommandSet, localCommandSet ist not set."); BashCommandSet bcs = new BashCommandSet(); - bcs.validate(); + bcs.validateShell(); return bcs; } @@ -1040,7 +1040,7 @@ public static ShellCommandSet getLocalCommandSet() { try { cls = (Class) LibrariesFactory.getGroovyClassLoader().loadClass(localCommandSet); ShellCommandSet shellCommandSet = cls.newInstance(); - if (shellCommandSet.validate()) + if (shellCommandSet.validateShell()) return shellCommandSet; throw new RuntimeException("The selected ShellCommandSet '${localCommandSet}' could not validate."); } catch (ClassNotFoundException e) { diff --git a/RoddyCore/src/de/dkfz/roddy/core/ExecutionContext.groovy b/RoddyCore/src/de/dkfz/roddy/core/ExecutionContext.groovy index 871a7f0d..c19c8df8 100644 --- a/RoddyCore/src/de/dkfz/roddy/core/ExecutionContext.groovy +++ b/RoddyCore/src/de/dkfz/roddy/core/ExecutionContext.groovy @@ -87,17 +87,17 @@ class ExecutionContext { * Keeps a list of errors which happen either on read back or on execution. * The list is not stored and rebuilt if necessary, so not all errors might be available. */ - private final List errors = [].asSynchronized() + private final List errors = ([] as List).asSynchronized() /** * Keeps a list of warnings which happen either on read back or on execution. * The list is not stored and rebuilt if necessary, so not all errors might be available. */ - private final List warnings = [].asSynchronized() + private final List warnings = ([] as List).asSynchronized() /** * Keeps a list of info entries which happen either on read back or on execution. * The list is not stored and rebuilt if necessary, so not all errors might be available. */ - private final List infos = [].asSynchronized() + private final List infos = ([] as List).asSynchronized() /** * The timestamp of this context object diff --git a/RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy b/RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy index b874f25a..17589102 100644 --- a/RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy +++ b/RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy @@ -92,7 +92,12 @@ abstract class ExecutionService implements BEExecutionService { ClassLoader classLoader = LibrariesFactory.getGroovyClassLoader() RunMode runMode = Roddy.getRunMode() - String executionServiceClassID = Roddy.applicationConfiguration.getOrSetApplicationProperty(runMode, Constants.APP_PROPERTY_EXECUTION_SERVICE_CLASS, SSHExecutionService.class.getName()) + String executionServiceClassID = + Roddy.applicationConfiguration. + getOrSetApplicationProperty( + runMode, + Constants.APP_PROPERTY_EXECUTION_SERVICE_CLASS, + SSHExecutionService.class.name) try { Class executionServiceClass = classLoader.loadClass(executionServiceClassID) initializeService(executionServiceClass, runMode) @@ -101,7 +106,10 @@ abstract class ExecutionService implements BEExecutionService { } } - ExecutionService() { + /** The constructor is not used except in the initialize method above. This is a singleton and the proper way to + * initialize it is to call the initialize method, not the constructor directly. + */ + protected ExecutionService() { } static ExecutionService getInstance() { @@ -221,9 +229,9 @@ abstract class ExecutionService implements BEExecutionService { return execute(cmd).resultLines } - ExecutionResult execute(String string, boolean waitFor = true, OutputStream outputStream = null) { - if (string) { - return _execute(string, waitFor, true, outputStream) + ExecutionResult execute(String command, boolean waitFor = true, OutputStream outputStream = null) { + if (command) { + return _execute(command, waitFor, true, outputStream) } else { return new ExecutionResult(false, -1, Arrays.asList("Command not valid. String is empty."), "") } @@ -759,9 +767,13 @@ abstract class ExecutionService implements BEExecutionService { //Check if the zip file exists. If so, uncompress it. if (provider.fileExists(remoteZipFile)) { // Unzip the file again. foundExisting stays true - GString str = RoddyIOHelperMethods.getCompressor().getDecompressionString(remoteZipFile, analysisToolsServerDir, analysisToolsServerDir) - getInstance().execute(str, true) - provider.setDefaultAccessRightsRecursively(new File(analysisToolsServerDir.getAbsolutePath()), context) + GString command = RoddyIOHelperMethods.compressor.getDecompressionString( + remoteZipFile, analysisToolsServerDir, analysisToolsServerDir) + instance.execute(command, true) + boolean success = provider. + setDefaultAccessRightsRecursively(new File(analysisToolsServerDir.absolutePath), context) + if (!success) + logger.warning("Ignoring error while setting default access rights on '${analysisToolsServerDir.absolutePath} (existing archive)'") } else { // Uh Oh, the file is not existing, the directory is not existing! Copy again and unzip foundExisting = false @@ -770,9 +782,8 @@ abstract class ExecutionService implements BEExecutionService { } if (foundExisting) { - //remoteFile.delete(); //Don't need that anymore analysisToolsServerDir = new File(dstCommonExecutionDirectory, "/dir_" + foundExisting) - logger.postSometimesInfo("Skipping copy of file ${remoteFile.getName()}, a file with the same md5 was found.") + logger.postSometimesInfo("Skipping copy of file ${remoteFile.name}, a file with the same md5 was found.") } else { analysisToolsServerDir = new File(dstCommonExecutionDirectory, "/dir_" + remoteFile.getName()) @@ -782,11 +793,17 @@ abstract class ExecutionService implements BEExecutionService { provider.checkFile(context.getFileForAnalysisToolsArchiveOverview(), true, context) provider.appendLineToFile(true, context.getFileForAnalysisToolsArchiveOverview(), "${remoteFile.getName()}:${archiveMD5}", true) - GString str = RoddyIOHelperMethods.getCompressor().getDecompressionString(new File(dstCommonExecutionDirectory, remoteFile.getName()), analysisToolsServerDir, analysisToolsServerDir) - getInstance().execute(str, true) - provider.setDefaultAccessRightsRecursively(new File(analysisToolsServerDir.getAbsolutePath()), context) + GString str = RoddyIOHelperMethods.compressor.getDecompressionString( + new File(dstCommonExecutionDirectory, remoteFile.getName()), + analysisToolsServerDir, analysisToolsServerDir) + instance.execute(str, true) + boolean success = provider. + setDefaultAccessRightsRecursively(new File(analysisToolsServerDir.absolutePath), context) + if (!success) + logger.warning("Ignoring error while setting default access rights on '${analysisToolsServerDir.absolutePath} (no existing archive)'") if (!provider.directoryExists(analysisToolsServerDir)) - context.addError(ExecutionContextError.EXECUTION_PATH_NOTFOUND.expand("The central archive ${analysisToolsServerDir.absolutePath} was not created!")) + context.addError(ExecutionContextError.EXECUTION_PATH_NOTFOUND. + expand("The central archive ${analysisToolsServerDir.absolutePath} was not created!")) } provider.checkDirectory(dstAnalysisToolsDirectory, context, true) diff --git a/RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy b/RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy index 6b7a0b85..48d233f7 100644 --- a/RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy +++ b/RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016 German Cancer Research Center (Deutsches Krebsforschungszentrum, DKFZ). + * Copyright (c) 2020 German Cancer Research Center (Deutsches Krebsforschungszentrum, DKFZ). * * Distributed under the MIT License (license terms are at https://www.github.com/TheRoddyWMS/Roddy/LICENSE.txt). */ @@ -10,16 +10,28 @@ import de.dkfz.roddy.SystemProperties import de.dkfz.roddy.config.Configuration import de.dkfz.roddy.config.ConfigurationConstants import de.dkfz.roddy.config.ConfigurationValue +import de.dkfz.roddy.execution.UnexpectedExecutionResultException import de.dkfz.roddy.tools.LoggerWrapper +import groovy.transform.CompileStatic + +import java.util.concurrent.CompletableFuture +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.Future +import java.util.function.Supplier /** * The local execution service executes commands on the local machine. For this groovy's execute() method is used. - * @author michael + * All commands are executed using Bash as interpreter! */ -@groovy.transform.CompileStatic +@CompileStatic class LocalExecutionService extends ExecutionService { + private static final LoggerWrapper logger = LoggerWrapper.getLogger(LocalExecutionService.class.name) + // Note that the thread pool is unbounded. + private ExecutorService executorService = Executors.newCachedThreadPool() + @Override String getUsername() { return SystemProperties.getUserName() @@ -38,25 +50,72 @@ class LocalExecutionService extends ExecutionService { return true } - // This method actually overrides a base class. But if we keep the @Override, the Groovy (or Java) compiler constantly - // claims, that the method does not override it's base method. - // That is, why we keep it in but only as a comment. -// @Override - protected ExecutionResult _execute(String command, boolean waitFor, boolean ignoreErrors, OutputStream outputStream = null) { + /** Read from an InputStream asynchronously using the executorService. + May throw an UncheckedIOException. + **/ + private CompletableFuture> asyncReadStringStream( + InputStream inputStream, + ExecutorService executorService = executorService) { + return CompletableFuture.supplyAsync({ + BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream)) + reader.lines().toArray() as List + } as Supplier>, executorService) + } + + /** + * @param command The command to execute. Note, this is always executed with Bash as the command interpreter. + * @param waitFor Whether to do synchronous (true) or asynchronous execution. Asynchronous execution + * will be done on the class-internal thread-pool. Don't do this with mutually blocking + * processes. + * @param ignoreErrors Ignored. No execution errors are reported. This is really nasty, but that's the way it + * is in the moment that doesn't break dependencies. Instead the method logs errors to the + * logging output. + * @param outputStream Ignored. + * @return + */ + @Override + protected ExecutionResult _execute(String command, + boolean waitFor, + boolean ignoreErrors, + OutputStream outputStream = null) { if (waitFor) { return LocalExecutionHelper.executeCommandWithExtendedResult(command, outputStream) } else { - Thread.start { - command.execute() - } - return new ExecutionResult(true, 0, [], "") + ProcessBuilder processBuilder = new ProcessBuilder(["bash", "-c", command]) + Process process = processBuilder.start() + Future> stdout = asyncReadStringStream(process.getInputStream()) + Future> stderr = asyncReadStringStream(process.getErrorStream()) + Future exitCode = CompletableFuture.supplyAsync({ + int exitCode = 0 + String message = null + try { + exitCode = process.waitFor() + if (exitCode != 0) { + message = ["Error executing command (exitCode=${exitCode}, command=${command}):", + "stdout={${stdout.get().join("\n")}}", + "stderr={${stderr.get().join("\n")}}"].join("\n") + } + } catch (InterruptedException e) { + message = "Interrupted command=${command}" + } + if (null != message) { + if (ignoreErrors) { + logger.postAlwaysInfo(message) + } else { + throw new UnexpectedExecutionResultException(message, []) + } + } + exitCode as Integer + } as Supplier, executorService) + + return new AsyncExecutionResult(exitCode, stdout, stderr) } } - @Override /** * Should be the current directory */ + @Override File queryWorkingDirectory() { return new File("") } diff --git a/RoddyCore/src/de/dkfz/roddy/execution/io/SSHExecutionService.groovy b/RoddyCore/src/de/dkfz/roddy/execution/io/SSHExecutionService.groovy index 6ff152c1..702c621b 100644 --- a/RoddyCore/src/de/dkfz/roddy/execution/io/SSHExecutionService.groovy +++ b/RoddyCore/src/de/dkfz/roddy/execution/io/SSHExecutionService.groovy @@ -363,18 +363,18 @@ class SSHExecutionService extends RemoteExecutionService { // That is, why we keep it in but only as a comment. // @Override synchronized ExecutionResult _execute(String command, boolean waitFor = true, boolean ignoreError = false, OutputStream outputStream = null) { - SSHPoolConnectionSet set = waitForService() - SSHClient sshClient = set.client + SSHPoolConnectionSet connectionSet = waitForService() + SSHClient sshClient = connectionSet.client if (waitFor) { - set.acquire() + connectionSet.acquire() Session session = sshClient.startSession() Session.Command cmd try { cmd = session.exec(command) } finally { - set.release() + connectionSet.release() } String content = readStream(cmd.getInputStream()) @@ -407,32 +407,32 @@ class SSHExecutionService extends RemoteExecutionService { } return new ExecutionResult(exitStatus == 0, exitStatus, output, "0") - } - - Runnable runnable = new Runnable() { - - @Override - void run() { -// long id = fireExecutionStartedEvent(command) - //Append a newgrp (Philip: better "newgrp -"!) to each command, so that all command context in the proper group context. - set.acquire() - Session session = sshClient.startSession() - Session.Command cmd - try { - cmd = session.exec(command) - } finally { - set.release() + } else { + Runnable runnable = new Runnable() { + + @Override + void run() { +// long id = fireExecutionStartedEvent(command) + // Append a newgrp (Philip: better "newgrp -"!) to each command, so that all command context in the proper group context. + connectionSet.acquire() + Session session = sshClient.startSession() + Session.Command cmd + try { + cmd = session.exec(command) + } finally { + connectionSet.release() + } + String content = IOUtils.readFully(cmd.getInputStream()).toString() + session.close() +// measureStop(id, "async command [sshclient:${set.id}] '" + RoddyIOHelperMethods.truncateCommand(command, Roddy.getOrSetApplicationProperty("commandLogTruncate", '20').toInteger()) + "'"); +// fireExecutionStoppedEvent(id, command) } - String content = IOUtils.readFully(cmd.getInputStream()).toString() - session.close() -// measureStop(id, "async command [sshclient:${set.id}] '" + RoddyIOHelperMethods.truncateCommand(command, Roddy.getOrSetApplicationProperty("commandLogTruncate", '20').toInteger()) + "'"); -// fireExecutionStoppedEvent(id, command) } + Thread thread = new Thread(runnable) + thread.setName("SSHExecutionService::_execute()") + thread.start() + return new ExecutionResult(true, 0, [], "") } - Thread thread = new Thread(runnable) - thread.setName("SSHExecutionService::_execute()") - thread.start() - return new ExecutionResult(true, 0, [], "") } // @Override diff --git a/RoddyCore/src/de/dkfz/roddy/execution/io/fs/BashCommandSet.groovy b/RoddyCore/src/de/dkfz/roddy/execution/io/fs/BashCommandSet.groovy index 4d2519cd..7a9fd7de 100644 --- a/RoddyCore/src/de/dkfz/roddy/execution/io/fs/BashCommandSet.groovy +++ b/RoddyCore/src/de/dkfz/roddy/execution/io/fs/BashCommandSet.groovy @@ -9,10 +9,11 @@ package de.dkfz.roddy.execution.io.fs import de.dkfz.roddy.config.converters.BashConverter import de.dkfz.roddy.config.converters.ConfigurationConverter import groovy.io.FileType -import org.apache.commons.io.filefilter.WildcardFileFilter /** * Provides a command generator for linux file systems / bash + * + * Please avoid using globs or filenames/paths with spaces, because most methods don't quote or escape the input. */ @groovy.transform.CompileStatic class BashCommandSet extends ShellCommandSet { @@ -28,15 +29,16 @@ class BashCommandSet extends ShellCommandSet { public static final String TRUE_OR_FALSE = "&& echo ${TRUE} || echo ${FALSE}" @Override - String getFileExistsTestCommand(File f) { - String path = f.getAbsolutePath() + String getFileExistsTestCommand(File file) { + String path = file.getAbsolutePath() return "[[ -f ${path} ]] " + TRUE_OR_FALSE } + @Override - String getDirectoryExistsTestCommand(File f) { - String path = f.getAbsolutePath() + String getDirectoryExistsTestCommand(File file) { + String path = file.getAbsolutePath() return "[[ -d ${path} ]]" + TRUE_OR_FALSE } @@ -80,8 +82,8 @@ class BashCommandSet extends ShellCommandSet { String getMyGroupCommand() { return "groups | cut -d \" \" -f 1" } @Override - String getOwnerOfPathCommand(File f) { - return "stat -c %U ${f.absolutePath}" + String getOwnerOfPathCommand(File file) { + return "stat -c %U ${file.absolutePath}" } @Override @@ -98,9 +100,15 @@ class BashCommandSet extends ShellCommandSet { return 'if [[ "${SET_PATH-}" != "" ]]; then export PATH=${SET_PATH}; fi' } + /** Get the numeric ID of a group. If group is non-existing (or null), then the command will fail with an exit + * code != 0. + * + * @param group + * @return + */ @Override - String getGroupIDCommand(String groupID) { - return "getent group ${groupID} | cut -d \":\" -f 3" + String getGroupIDCommand(String group) { + return "getent group ${group} | cut -d \":\" -f 3" } @Override @@ -122,36 +130,83 @@ class BashCommandSet extends ShellCommandSet { return getCheckAndCreateDirectoryCommand(file, onCreateFileGroup, onCreateAccessRights) + TRUE_OR_FALSE; } - @Override - String getCheckAndCreateDirectoryCommand(File f, String onCreateAccessRights, String onCreateFileGroup) { - String path = f.absolutePath + /** If onCreateAccessRights and onCreateFileGroup are both != null, change to the group and create the path + * using the group and access rights. However, if the path already exists (file or whatever), do nothing + * (`umask $mask && mkdir -p $path` does nothing on an existing directory) or fail, if the path is occupied + * by a file rather than a directory. + * + * If one of onCreateAccessRights and onCreateFileGroup is null, do nothing if the file/path exists. If the + * file does not exist, create a directory. If the directory creation fails, **ignore the error**. + * + * @param file + * @param onCreateAccessRights + * @param onCreateFileGroup + * @return + */ + @Override + String getCheckAndCreateDirectoryCommand(File file, String onCreateAccessRights, String onCreateFileGroup) { + String path = file.absolutePath String checkExistence = "[[ -e ${path} ]]" if (onCreateAccessRights && onCreateFileGroup) return "sg ${onCreateFileGroup} -c \"${checkExistence} || umask ${onCreateAccessRights} && mkdir -p ${path}\"" else + // I do not understand, why this command ignores execution errors (via `|| echo ''`). return "${checkExistence} || install -d \"${path}\" || echo ''" } - @Override - String getCheckChangeOfPermissionsPossibilityCommand(File f, String group) { - File testFile = new File(f, ".roddyPermissionsTestFile") - return "(touch ${testFile}; chmod u+rw ${testFile} &> /dev/null && chgrp ${group} ${testFile} &> /dev/null) $TRUE_OR_FALSE; rm ${testFile} 2>/dev/null; echo ''" + /** Check whether it is possible to change permissions at the target-location. Specifically, the path `file` will + * be appended by ".roddyPermissionTestFile" and it is checked, whether its permissions can get changed. + * + * Currently, it is just checked whether chmod is working on that file and -- if group is not null -- whether + * the group ownership can be changed to the target group. + * + * @param file path to check. Not exactly this path is changed, but a file besides that path. + * @param group target-group to check; if this is null, then only chmod is tested, otherwise also chgrp. + * @return + */ + @Override + String getCheckChangeOfPermissionsPossibilityCommand(File file, String group) { + File testFile = new File(file, ".roddyPermissionsTestFile") + if (null == group) { + return "(touch ${testFile}; chmod u+rw ${testFile} &> /dev/null) $TRUE_OR_FALSE; rm ${testFile} 2>/dev/null; echo ''" + } else { + return "(touch ${testFile}; chmod u+rw ${testFile} &> /dev/null && chgrp ${group} ${testFile} &> /dev/null) $TRUE_OR_FALSE; rm ${testFile} 2>/dev/null; echo ''" + } } @Override - String getSetAccessRightsCommand(File f, String rightsForFiles, String fileGroup) { - return "chmod ${rightsForFiles} ${f.absolutePath}; chgrp ${fileGroup} ${f.absolutePath}" + Optional getSetAccessRightsCommand(File f, String rightsForFiles, String fileGroup) { + def path = "${f.absolutePath}" + String result = "" + if (null != rightsForFiles) + result += "chmod ${rightsForFiles} ${path}; " + if (null != fileGroup) + result += "chgrp ${fileGroup} ${path}" + return Optional.ofNullable(result == "" ? null : result) } @Override - String getSetAccessRightsRecursivelyCommand(File f, String rightsForDirectories, String rightsForFiles, String fileGroup) { - def path = "${f.getAbsolutePath()}" - return "find ${path} -type d | xargs chmod ${rightsForDirectories}; find ${path} -type d | xargs chgrp ${fileGroup}; find ${path} -type f | xargs chmod ${rightsForFiles}; find ${path} -type f | xargs chgrp ${fileGroup};" + Optional getSetAccessRightsRecursivelyCommand(File f, String rightsForDirectories, String rightsForFiles, String fileGroup) { + def path = "${f.absolutePath}" + String result = "" + if (null != fileGroup) + result += "find ${path} -type d -or type f | xargs chgrp ${fileGroup}; " + if (null != rightsForDirectories) + result += "find ${path} -type d | xargs chmod ${rightsForDirectories}; " + if (null != rightsForFiles) + result += "find ${path} -type f | xargs chmod ${rightsForFiles}; " + return Optional.ofNullable(result == "" ? null : result) } + /** This is pretty much business logic that should no be in the BashCommandSet just for the reason that a + * Bash command is used. + * + * @param f + * @return + */ @Override String getCheckCreateAndReadoutExecCacheFileCommand(File f) { - return "[[ ! -e ${f.absolutePath} ]] && find ${f.parent} -mindepth 1 -maxdepth 2 -name exec_* > ${f.absolutePath}; cat ${f.absolutePath}" + return "[[ ! -e ${f.absolutePath} ]] && find ${f.absoluteFile.parent} -mindepth 1 -maxdepth 2 -name exec_* > ${f.absolutePath}; cat ${f.absolutePath}" } @Override @@ -161,13 +216,13 @@ class BashCommandSet extends ShellCommandSet { @Override String getReadLineOfFileCommand(File file, int lineIndex) { - return "tail -n +${lineIndex + 1} ${file.getAbsolutePath()} | head -n 1" + return "tail -n +${lineIndex + 1} ${file.absolutePath} | head -n 1" } -/** - * Creates a list of all directories in a directory. - * @param f - * @return - */ + /** + * Creates a list of all directories in a directory. + * @param f + * @return + */ @Override String getListDirectoriesInDirectoryCommand(File f) { return "ls -lL ${f.absolutePath} 2> /dev/null | grep '^d' | awk '{ print \$9 }' | uniq" @@ -255,20 +310,23 @@ class BashCommandSet extends ShellCommandSet { @Override String getCopyFileCommand(File _in, File _out) { - return "cp -p ${_in.getAbsolutePath()} ${_out.getAbsolutePath()}" + return "cp -p ${_in.absolutePath} ${_out.absolutePath}" } @Override String getCopyDirectoryCommand(File _in, File _out) { - return "cp -pr ${_in.getAbsolutePath()} ${_out.getAbsolutePath()}" + return "cp -pr ${_in.absolutePath} ${_out.absolutePath}" } @Override - String getMoveFileCommand(File _in, File _out) { return "mv ${_in.getAbsolutePath()} ${_out.getAbsolutePath()}" } + String getMoveFileCommand(File _in, File _out) { + return "mv ${_in.absolutePath} ${_out.absolutePath}" \ + } @Override String getLockedAppendLineToFileCommand(File file, String line) { - return "lockfile ${file}~; echo \"${line}\" >> ${file}; rm -rf ${file}~" + String path = file.absolutePath + return "lockfile ${path}~; echo \"${line}\" >> ${path}; rm -rf ${path}~" } @Override @@ -283,12 +341,12 @@ class BashCommandSet extends ShellCommandSet { @Override String getRemoveDirectoryCommand(File directory) { - return "rm -rf ${directory.getAbsolutePath()}" + return "rm -rf ${directory.absolutePath}" } @Override String getRemoveFileCommand(File file) { - return "rm -f ${file.getAbsolutePath()}" + return "rm -f ${file.absolutePath}" } @Override @@ -317,13 +375,13 @@ class BashCommandSet extends ShellCommandSet { } @Override - boolean validate() { + boolean validateShell() { def file = new File("/bin/bash") return file.exists() && file.canExecute() } @Override String getFileSizeCommand(File file) { - return "stat --printf='%s' '${file}'" + return "stat --printf='%s' ${file.absolutePath}" } } diff --git a/RoddyCore/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProvider.groovy b/RoddyCore/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProvider.groovy index 47718f8e..efff8d81 100644 --- a/RoddyCore/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProvider.groovy +++ b/RoddyCore/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProvider.groovy @@ -622,15 +622,21 @@ class FileSystemAccessProvider { } /** - * Sets the rights for all files in the path and ints subpaths. - * @param path - * @param accessString + * Sets the rights for all files in the path and ints subpaths. Return true if the executed command finished + * successfully, otherwise false. Only access rights that are provided as non-null strings are actually set. + * If all three access right strings are null, the method returns true! + * Permission strings must be valid for chmod. The group must exist. If an error occurs, false is returned. + * + * @param path path to change access rights (recursively) + * @param accessStringDirectories permission string for directories (string, such as "rwx", or octal) + * @param accessStringFiles permission string for files (string, such as "rwx", or octal) + * @param group group name * @return */ - boolean setAccessRightsRecursively(File path, String accessStringDirectories, String accessString, String group) { - return ExecutionService.instance. - execute(commandSet.getSetAccessRightsRecursivelyCommand( - path, accessStringDirectories, accessString, group), false) + boolean setAccessRightsRecursively(File path, String accessStringDirectories, String accessStringFiles, String group) { + commandSet.getSetAccessRightsRecursivelyCommand(path, accessStringDirectories, accessStringFiles, group) + .map { ExecutionService.instance.execute(it, false).successful } + .orElse(true) } boolean setDefaultAccessRights(File file, ExecutionContext context) { @@ -639,12 +645,25 @@ class FileSystemAccessProvider { return setAccessRights(file, context.outputFileAccessRights, context.outputGroupString) } - boolean setAccessRights(File file, String accessString, String groupID) { + /** + * Sets the rights for a file. Return true if the executed command finished successfully, otherwise false. + * Only access rights that are provided as non-null strings are actually set. If all three access right strings + * are null, the method returns true! Permission strings must be valid for chmod. The group must exist. If an error + * occurs, false is returned. + * + * @param path fil to change access rights + * @param accessString permission string for file (string, such as "rwx", or octal) + * @param group group name + * @return + */ + boolean setAccessRights(File file, String accessString, String group) { ExecutionService eService = ExecutionService.instance if (eService.canModifyAccessRights()) { - return eService.modifyAccessRights(file, accessString, groupID) + return eService.modifyAccessRights(file, accessString, group) } else { - return eService.execute(commandSet.getSetAccessRightsCommand(file, accessString, groupID)) + return commandSet.getSetAccessRightsCommand(file, accessString, group) + .map { eService.execute(it).successful } + .orElse(true) } } diff --git a/RoddyCore/src/de/dkfz/roddy/execution/io/fs/NoNoFileSystemAccessProvider.java b/RoddyCore/src/de/dkfz/roddy/execution/io/fs/NoNoFileSystemAccessProvider.java index 1e30daaa..c4c475eb 100644 --- a/RoddyCore/src/de/dkfz/roddy/execution/io/fs/NoNoFileSystemAccessProvider.java +++ b/RoddyCore/src/de/dkfz/roddy/execution/io/fs/NoNoFileSystemAccessProvider.java @@ -101,7 +101,7 @@ public boolean copyDirectory(File _in, File _out) { } @Override - public boolean setAccessRights(File file, String accessString, String groupID) { + public boolean setAccessRights(File file, String accessString, String group) { return true; } diff --git a/RoddyCore/src/de/dkfz/roddy/execution/io/fs/ShellCommandSet.groovy b/RoddyCore/src/de/dkfz/roddy/execution/io/fs/ShellCommandSet.groovy index aeba3240..6d503731 100644 --- a/RoddyCore/src/de/dkfz/roddy/execution/io/fs/ShellCommandSet.groovy +++ b/RoddyCore/src/de/dkfz/roddy/execution/io/fs/ShellCommandSet.groovy @@ -52,9 +52,9 @@ abstract class ShellCommandSet { abstract String getCheckChangeOfPermissionsPossibilityCommand(File f, String group) - abstract String getSetAccessRightsCommand(File f, String rightsForFiles, String fileGroup) + abstract Optional getSetAccessRightsCommand(File f, String rightsForFiles, String fileGroup) - abstract String getSetAccessRightsRecursivelyCommand(File f, String rightsForDirectories, String rightsForFiles, String fileGroup) + abstract Optional getSetAccessRightsRecursivelyCommand(File f, String rightsForDirectories, String rightsForFiles, String fileGroup) abstract String getCheckCreateAndReadoutExecCacheFileCommand(File f) @@ -139,7 +139,7 @@ abstract class ShellCommandSet { abstract List getShellExecuteCommand(String... commands) - abstract boolean validate() + abstract boolean validateShell() abstract String getFileSizeCommand(File file) diff --git a/RoddyCore/src/de/dkfz/roddy/execution/jobs/Job.groovy b/RoddyCore/src/de/dkfz/roddy/execution/jobs/Job.groovy index 6b29e60d..6f3d2633 100644 --- a/RoddyCore/src/de/dkfz/roddy/execution/jobs/Job.groovy +++ b/RoddyCore/src/de/dkfz/roddy/execution/jobs/Job.groovy @@ -172,7 +172,7 @@ class Job extends BEJob { , inlineScript ? null : getToolMD5(TOOLID_WRAPIN_SCRIPT, context) , getResourceSetFromConfiguration(toolID, context) , [] - , [:] + , [:] as Map , Roddy.getJobManager() , JobLog.toOneFile(new File(context.loggingDirectory, jobName + ".o{JOB_ID}")) , null) diff --git a/RoddyCore/src/de/dkfz/roddy/plugins/LibrariesFactory.groovy b/RoddyCore/src/de/dkfz/roddy/plugins/LibrariesFactory.groovy index 64cf04c9..adc38351 100644 --- a/RoddyCore/src/de/dkfz/roddy/plugins/LibrariesFactory.groovy +++ b/RoddyCore/src/de/dkfz/roddy/plugins/LibrariesFactory.groovy @@ -56,7 +56,7 @@ class LibrariesFactory extends Initializable { private Map loadedJarsByPlugin = [:] - private PluginInfoMap mapOfPlugins = [:]; + private PluginInfoMap mapOfPlugins = new PluginInfoMap([:]) private static final Map> mapOfErrorsForPluginEntries = [:] @@ -518,7 +518,7 @@ class LibrariesFactory extends Initializable { usedPluginsCorrected << [id, fullVersion].join(":"); return new Tuple2(id, fullVersion); } as List>; - usedPlugins = usedPluginsCorrected; + usedPlugins = usedPluginsCorrected as String[] Map pluginsToActivate = [:]; while (pluginsToCheck.size() > 0) { diff --git a/RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy b/RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy new file mode 100644 index 00000000..1ee170ba --- /dev/null +++ b/RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy @@ -0,0 +1,81 @@ +package de.dkfz.roddy.execution.io + +import de.dkfz.roddy.RunMode +import de.dkfz.roddy.core.ContextResource +import org.junit.Rule +import spock.lang.Specification + +class LocalExecutionServiceSpec extends Specification { + + @Rule + final ContextResource contextResource = new ContextResource() + + static { + ExecutionService.initializeService(LocalExecutionService, RunMode.CLI) + } + + LocalExecutionService es = ExecutionService.instance + + def "execute synchronously and succeed with captured stdout and non!-ignored stderr"() { + when: + ExecutionResult res = es.execute("echo 'hello'; echo 'error' >> /dev/stderr", true) + + then: + res.exitCode == 0 + res.successful + res.resultLines.size() == 2 + // The order of stderr and stdout is not predictable in the current implementation in RoddyToolLib. + // Note in rare cases (1/30 testruns) a "hello" followed by 5 null values is printed an the test fails. + System.err.println(res.resultLines.join(", ")) + (res.resultLines[0] == "hello" && res.resultLines[1] == "error") || + (res.resultLines[0] == "error" && res.resultLines[1] == "hello") || + res.resultLines[0] == "helloerror" || + res.resultLines[0] == "errorhello" + } + + def "execute synchronously and fail with captured stdout and non!-ignored stderr"() { + when: + ExecutionResult res = es.execute("echo 'hello'; echo 'error' >> /dev/stderr; false", true) + + then: + res.exitCode == 1 + !res.successful + res.resultLines.size() == 2 + // The order of stderr and stdout is not predictable in the current implementation in RoddyToolLib. + (res.resultLines[0] == "hello" && res.resultLines[1] == "error") \ + || (res.resultLines[0] == "error" && res.resultLines[1] == "hello") + } + + def "execute synchronously and fail with empty stdout and non!-ignored stderr"() { + when: + ExecutionResult res = es.execute("echo 'error' >> /dev/stderr; false", true) + + then: + res.exitCode == 1 + !res.successful + res.resultLines.size() == 1 + res.resultLines[0] == "error" + } + + def "execute asynchronously and fail with captured stdout but ignored! stderr"() { + when: + ExecutionResult res = es.execute("echo 'hello'; echo 'error' >> /dev/stderr; false", false) + + then: + res.exitCode == 1 + !res.successful + res.resultLines.size() == 1 + res.resultLines[0] == "hello" // Note that the newline is removed. + } + + def "execute asynchronously and succeed with empty stdout and ignored! stderr"() { + when: + ExecutionResult res = es.execute("sleep 2; echo 'error' >> /dev/stderr; true", false) + + then: + res.exitCode == 0 + res.successful + res.resultLines.size() == 0 + } + +} diff --git a/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetSpecification.groovy b/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetSpec.groovy similarity index 98% rename from RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetSpecification.groovy rename to RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetSpec.groovy index 6aae0c49..3ebe2347 100644 --- a/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetSpecification.groovy +++ b/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetSpec.groovy @@ -8,7 +8,7 @@ import static groovy.io.FileType.ANY import static groovy.io.FileType.DIRECTORIES import static groovy.io.FileType.FILES -class BashCommandSetSpecification extends Specification { +class BashCommandSetSpec extends Specification { @Shared BashCommandSet b = new BashCommandSet() diff --git a/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetTest.groovy b/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetTest.groovy index d50eefa2..38840403 100644 --- a/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetTest.groovy +++ b/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/BashCommandSetTest.groovy @@ -44,6 +44,6 @@ class BashCommandSetTest { // This method can only be tested if /bin/bash is really avaible. The test makes no sense in my oppinion def file = new File("/bin/bash") - assert file.canRead() && file.canExecute() == new BashCommandSet().validate() + assert file.canRead() && file.canExecute() == new BashCommandSet().validateShell() } } diff --git a/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProviderSpecification.groovy b/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProviderSpec.groovy similarity index 97% rename from RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProviderSpecification.groovy rename to RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProviderSpec.groovy index ff66f36b..2e177300 100644 --- a/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProviderSpecification.groovy +++ b/RoddyCore/test/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProviderSpec.groovy @@ -12,7 +12,7 @@ import static de.dkfz.roddy.execution.io.fs.FileSystemAccessProvider.RegexSearch import static de.dkfz.roddy.execution.io.fs.FileSystemAccessProvider.RegexSearchDepth.AbsolutePath import static de.dkfz.roddy.execution.io.fs.FileSystemAccessProvider.RegexSearchDepth.RelativeToSearchFolder -class FileSystemAccessProviderSpecification extends Specification { +class FileSystemAccessProviderSpec extends Specification { @ClassRule static ContextResource contextResource = new ContextResource() diff --git a/build.gradle b/build.gradle index e3c4cc1e..dc269970 100755 --- a/build.gradle +++ b/build.gradle @@ -91,8 +91,8 @@ repositories { maven { url "https://repo1.maven.org/maven2/" } // Some of the remaining dependencies } - -ext.roddyToolLibVersion = "2.1.1" +// TODO For testing only. Needs to be changed as soon as RoddyToolLib is released. +ext.roddyToolLibVersion = "2.2.2" configurations.all { resolutionStrategy.cacheChangingModulesFor(0, 'seconds')