Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Nextflow compatible with NixOS (or other systems without /bin/bash) #5432

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class BashWrapperBuilder {

static final public List<String> BASH
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to rename BASH to BIN_BASH to make the difference clear.


static final public List<String> ENV_BASH

static private int level

@PackageScope
Expand All @@ -85,6 +87,7 @@ class BashWrapperBuilder {
log.warn "Invalid value for `NXF_DEBUG` variable: $str -- See http://www.nextflow.io/docs/latest/config.html#environment-variables"
}
BASH = Collections.unmodifiableList( level > 0 ? ['/bin/bash','-uex'] : ['/bin/bash','-ue'] )
ENV_BASH = Collections.unmodifiableList(['/usr/bin/env', '-S', 'bash', BASH[1]])

}

Expand Down Expand Up @@ -571,7 +574,8 @@ class BashWrapperBuilder {
final traceWrapper = isTraceRequired()
if( traceWrapper ) {
// executes the stub which in turn executes the target command
launcher = "/bin/bash ${fileStr(wrapperFile)} nxf_trace"
String traceInterpreter = runWithContainer ? "/bin/bash" : "/usr/bin/env bash"
launcher = "${traceInterpreter} ${fileStr(wrapperFile)} nxf_trace"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No -S here becuase bash does not get any further options (think -ue)

}
else {
launcher = "${interpreter} ${fileStr(scriptFile)}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class LocalTaskHandler extends TaskHandler implements FusionAwareTask {
}

protected ProcessBuilder localProcessBuilder() {
final cmd = new ArrayList<String>(BashWrapperBuilder.BASH) << wrapperFile.getName()
final cmd = new ArrayList<String>(BashWrapperBuilder.ENV_BASH) << wrapperFile.getName()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sure that .command.run gets executed correctly on a host without /bin/bash.

log.debug "Launch cmd line: ${cmd.join(' ')}"
// make sure it's a posix file system
if( task.workDir.fileSystem != FileSystems.default )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ class TaskBean implements Serializable, Cloneable {
this.condaEnv = task.getCondaEnv()
this.spackEnv = task.getSpackEnv()
this.moduleNames = task.config.getModule()
this.shell = task.config.getShell() ?: BashWrapperBuilder.BASH
this.shell = task.config.getShell(task.isContainerEnabled()) ?: BashWrapperBuilder.BASH
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, task will always (?) have a shell since it comes with the process defaults. I have not toched the "else"-block here since I think it is actually not necessary (and obviously did not cause issues for me).

this.script = task.getScript()
this.beforeScript = task.config.getBeforeScript()
this.afterScript = task.config.getAfterScript()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,10 @@ class TaskConfig extends LazyMap implements Cloneable {
return (List<String>) get('secret')
}

List<String> getShell() {
List<String> getShell(boolean isContainerEnabled = true) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to adapt the unit tests but I don't know how 🤷 .

final value = get('shell')
if( !value )
return BashWrapperBuilder.BASH
return isContainerEnabled ? BashWrapperBuilder.BASH : BashWrapperBuilder.ENV_BASH

if( value instanceof List )
return (List)value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ class ProcessConfig implements Map<String,Object>, Cloneable {
static final Map<String,Object> DEFAULT_CONFIG = [
debug: false,
cacheable: true,
shell: BashWrapperBuilder.BASH,
maxRetries: 0,
maxErrors: -1,
errorStrategy: ErrorStrategy.TERMINATE
Expand Down Expand Up @@ -174,14 +173,15 @@ class ProcessConfig implements Map<String,Object>, Cloneable {
*
* @param script The owner {@code BaseScript} configuration object
*/
protected ProcessConfig( BaseScript script ) {
protected ProcessConfig( BaseScript script, boolean isContainerEnabled = true) {
ownerScript = script
configProperties = new LinkedHashMap()
configProperties.putAll( DEFAULT_CONFIG )
configProperties.put('shell', isContainerEnabled ? BashWrapperBuilder.BASH : BashWrapperBuilder.ENV_BASH)
}

ProcessConfig( BaseScript script, String name ) {
this(script)
ProcessConfig( BaseScript script, String name, boolean isContainerEnabled = true) {
this(script, isContainerEnabled)
this.processName = name
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class ProcessDef extends BindableDef implements IterableDef, ChainableDef {
assert processConfig==null

// the config object
processConfig = new ProcessConfig(owner,processName)
processConfig = new ProcessConfig(owner,processName,session.getContainerConfig().isEnabled())

// Invoke the code block which will return the script closure to the executed.
// As side effect will set all the property declarations in the 'taskConfig' object.
Expand Down