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.

Sign up for GitHub

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 Bash path configurable #5696

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Make Bash path configurable #5696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55d191a
Make Bash path configurable
pditommaso Jan 22, 2025
c0e2aa7
Fix tests
pditommaso Jan 22, 2025
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
6 changes: 6 additions & 0 deletions modules/nextflow/src/main/groovy/nextflow/NF.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package nextflow

import groovy.runtime.metaclass.NextflowDelegatingMetaClass
import groovy.transform.Memoized
import nextflow.extension.CH
import nextflow.plugin.extension.PluginExtensionProvider
import nextflow.script.ExecutionStack
Expand Down Expand Up @@ -79,4 +80,9 @@ class NF {
static boolean isTopicChannelEnabled() {
NextflowMeta.instance.preview.topic
}

@Memoized
static String bash() {
session().config.navigate('nextflow.defaults.bash') ?: '/bin/bash'
Copy link
Member

Choose a reason for hiding this comment

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

This config scope was renamed to workflow, I would just call it workflow.bash

Copy link
Member Author

Choose a reason for hiding this comment

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

let's discuss about this. I think we need to nextflow wide setting (more tied to runtime than workflow concept)

}
}
10 changes: 5 additions & 5 deletions modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import java.nio.file.Path
import groovy.transform.CompileStatic
import groovy.transform.PackageScope
import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.SysEnv
import nextflow.container.ContainerBuilder
import nextflow.container.DockerBuilder
Expand Down Expand Up @@ -84,8 +85,7 @@ class BashWrapperBuilder {
catch( Exception e ) {
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'] )

BASH = Collections.unmodifiableList( level > 0 ? [NF.bash(), '-uex'] : [NF.bash(), '-ue'] )
Comment on lines -87 to +88
Copy link
Member

Choose a reason for hiding this comment

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

This use of NF.bash() makes me think that the user bash setting can only be the bash path itself and can't include extra arguments like env -S bash or bash -o pipefail. Will that be acceptable?

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal should be to specify a custom Bash path not to alter the shell option. That's possible in some extent via process.shell

Copy link

Choose a reason for hiding this comment

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

My understanding is that BASH is used for both running the task on the host as well as within the container. This means NF.bash should be something that is acceptable in both situations (OR: one has to change the profile and NF.bash at the same time if one needs to use different interpreters depending on whether the task runs on a host or on a container). I also understand that /bin/bash is the only official entry point to run docker containers.

Maybe for some context, here is a test script that I am using to check whether the Nextflow package is running as expected on NixOS. I'm linking this here to show what my goal is/was with #5684. (The test is easy to understand without knowledge of Nix.). 5684 would allow me to pass this test.

I think even today, I could use an unpatched Nextflow on NixOS if I "simply" adapt process.shell everywhere. Tracing might not work. I'd like to improve on that situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

This means NF.bash should be something that is acceptable in both situations

Yes, that's correct.

I think even today, I could use an unpatched Nextflow on NixOS if I "simply" adapt process.shell everywhere. Tracing might not work. I'd like to improve on that situation.

my understanding is that in NixOS the Bash interpreter is located at /usr/bin/bash so this PR should solve the problem with a minimal setting.

Is it not the case?

Copy link

Choose a reason for hiding this comment

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

NixOS has bash at /run/current-system/sw/bin/bash but the correct way to invoke it is /usr/bin/env -S bash.

But: The docker images doesn't have it there. They have it (officially) at /bin/bash. My whole point is that I want the system to not use the same (path to) bash in both cases (docker execution vs host execution). Let me write a another comment to explain the situation below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. What's preventing creating a symlink to "normalise" it?

Copy link

Choose a reason for hiding this comment

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

A Nix package cannot create arbitrary files on the file system. Actually, older definitions of the Nextflow package used a wrapper to make nextflow believe that /bin/bash exists. However, using this wrapper comes with other drawbacks.

}


Expand Down Expand Up @@ -567,7 +567,7 @@ class BashWrapperBuilder {
// keep the shell path as "/bin/bash" when a non-custom "shell" attribute is specified
// to not introduce unexpected changes due to the fact BASH is defined as "/bin/bash -eu" by default
return shell.is(BASH)
? "/bin/bash"
? NF.bash()
: shell.join(' ')
}

Expand Down Expand Up @@ -600,7 +600,7 @@ class BashWrapperBuilder {
if( (env || needChangeTaskWorkDir) && !containerConfig.entrypointOverride() ) {
if( needChangeTaskWorkDir )
cmd = 'cd $NXF_TASK_WORKDIR; ' + cmd
cmd = "/bin/bash -c \"$cmd\""
cmd = "${NF.bash()} -c \"$cmd\""
}
launcher = containerBuilder.getRunCommand(cmd)
}
Expand Down Expand Up @@ -738,7 +738,7 @@ class BashWrapperBuilder {
builder.params(readOnlyInputs: true)

if( this.containerConfig.entrypointOverride() )
builder.params(entry: '/bin/bash')
builder.params(entry: NF.bash())

// give a chance to override any option with process specific `containerOptions`
if( containerOptions ) {
Expand Down
3 changes: 2 additions & 1 deletion modules/nextflow/src/main/groovy/nextflow/executor/FluxExecutor.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import java.util.regex.Pattern

import groovy.transform.CompileStatic
import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.processor.TaskConfig
import nextflow.processor.TaskRun
/**
Expand Down Expand Up @@ -93,7 +94,7 @@ class FluxExecutor extends AbstractGridExecutor {
// Any extra cluster options the user wants!
addClusterOptionsDirective(task.config, result)

result << '/bin/bash' << scriptFile.getName()
result << NF.bash() << scriptFile.getName()
return result
}

Expand Down
3 changes: 2 additions & 1 deletion modules/nextflow/src/main/groovy/nextflow/executor/GridTaskHandler.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import dev.failsafe.function.CheckedSupplier
import groovy.transform.CompileStatic
import groovy.transform.Memoized
import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.exception.ProcessException
import nextflow.exception.ProcessFailedException
import nextflow.exception.ProcessNonZeroExitStatusException
Expand Down Expand Up @@ -231,7 +232,7 @@ class GridTaskHandler extends TaskHandler implements FusionAwareTask {
final containerOpts = task.config.getContainerOptions()
final cmd = FusionHelper.runWithContainer(launcher, config, task.getContainer(), containerOpts, submit)
// create an inline script to launch the job execution
return '#!/bin/bash\n' + submitDirective(task) + cmd + '\n'
return '#!'+ NF.bash()+'\n' + submitDirective(task) + cmd + '\n'
}

protected String submitDirective(TaskRun task) {
Expand Down
5 changes: 3 additions & 2 deletions modules/nextflow/src/main/groovy/nextflow/k8s/K8sDriverLauncher.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import com.beust.jcommander.DynamicParameter
import com.beust.jcommander.Parameter
import com.google.common.hash.Hashing
import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.cli.CmdKubeRun
import nextflow.cli.CmdRun
import nextflow.config.ConfigBuilder
Expand Down Expand Up @@ -535,7 +536,7 @@ class K8sDriverLauncher {
PodSpecBuilder builder = new PodSpecBuilder()
.withPodName(runName)
.withImageName(headImage ?: k8sConfig.getNextflowImageName())
.withCommand(['/bin/bash', '-c', cmd])
.withCommand([NF.bash(), '-c', cmd])
.withLabels([ app: 'nextflow', runName: runName ])
.withNamespace(k8sClient.config.namespace)
.withServiceAccount(k8sClient.config.serviceAccount)
Expand Down Expand Up @@ -692,7 +693,7 @@ class K8sDriverLauncher {

protected void launchLogin() {
def launchDir = k8sConfig.getLaunchDir()
def cmd = "kubectl -n ${k8sClient.config.namespace} exec -it $runName -- /bin/bash -c 'cd $launchDir; exec bash --login -i'"
def cmd = "kubectl -n ${k8sClient.config.namespace} exec -it $runName -- ${NF.bash()} -c 'cd $launchDir; exec bash --login -i'"
def proc = new ProcessBuilder().command('bash','-c',cmd).inheritIO().start()
def result = proc.waitFor()
if( result == 0 ) {
Expand Down
5 changes: 3 additions & 2 deletions modules/nextflow/src/main/groovy/nextflow/k8s/model/PodSpecBuilder.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import java.util.concurrent.atomic.AtomicInteger
import groovy.transform.CompileDynamic
import groovy.transform.CompileStatic
import groovy.transform.PackageScope
import nextflow.NF
import nextflow.executor.res.AcceleratorResource
import nextflow.util.MemoryUnit
import groovy.util.logging.Slf4j
Expand Down Expand Up @@ -169,14 +170,14 @@ class PodSpecBuilder {
PodSpecBuilder withCommand( cmd ) {
if( cmd==null ) return this
assert cmd instanceof List || cmd instanceof CharSequence, "Missing or invalid K8s command parameter: $cmd"
this.command = cmd instanceof List ? cmd as List<String> : ['/bin/bash','-c', cmd.toString()]
this.command = cmd instanceof List ? cmd as List<String> : [NF.bash(), '-c', cmd.toString()]
return this
}

PodSpecBuilder withArgs( args ) {
if( args==null ) return this
assert args instanceof List || args instanceof CharSequence, "Missing or invalid K8s args parameter: $args"
this.args = args instanceof List ? args as List<String> : ['/bin/bash','-c', args.toString()]
this.args = args instanceof List ? args as List<String> : [NF.bash(), '-c', args.toString()]
return this
}

Expand Down
3 changes: 2 additions & 1 deletion plugins/nf-google/src/main/nextflow/cloud/google/batch/GoogleBatchExecutor.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import java.nio.file.Path
import com.google.cloud.storage.contrib.nio.CloudStoragePath
import groovy.transform.CompileStatic
import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.SysEnv
import nextflow.cloud.google.batch.client.BatchClient
import nextflow.cloud.google.batch.client.BatchConfig
Expand Down Expand Up @@ -177,7 +178,7 @@ class GoogleBatchExecutor extends Executor implements ExtensionPoint, TaskArrayE
return TaskArrayExecutor.super.getArrayLaunchCommand(taskDir)
}
else {
final cmd = List.of('/bin/bash','-o','pipefail','-c', launchCommand(taskDir))
final cmd = List.of(NF.bash(),'-o','pipefail','-c', launchCommand(taskDir))
return Escape.cli(cmd as String[])
}
}
Expand Down
3 changes: 2 additions & 1 deletion plugins/nf-google/src/main/nextflow/cloud/google/batch/GoogleBatchScriptLauncher.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.google.cloud.batch.v1.Volume
import com.google.cloud.storage.contrib.nio.CloudStoragePath
import groovy.transform.CompileStatic
import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.cloud.google.batch.client.BatchConfig
import nextflow.executor.BashWrapperBuilder
import nextflow.extension.FilesEx
Expand Down Expand Up @@ -178,7 +179,7 @@ class GoogleBatchScriptLauncher extends BashWrapperBuilder implements GoogleBatc
}

static String launchCommand( String workDir ) {
"trap \"{ cp ${TaskRun.CMD_LOG} ${workDir}/${TaskRun.CMD_LOG}; }\" ERR; /bin/bash ${workDir}/${TaskRun.CMD_RUN} 2>&1 | tee ${TaskRun.CMD_LOG}"
"trap \"{ cp ${TaskRun.CMD_LOG} ${workDir}/${TaskRun.CMD_LOG}; }\" ERR; ${NF.bash()} ${workDir}/${TaskRun.CMD_RUN} 2>&1 | tee ${TaskRun.CMD_LOG}"
}

static String containerMountPath(CloudStoragePath path) {
Expand Down
3 changes: 2 additions & 1 deletion ...ins/nf-google/src/main/nextflow/cloud/google/lifesciences/GoogleLifeSciencesConfig.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import groovy.transform.CompileStatic
import groovy.transform.Memoized
import groovy.transform.ToString
import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.Session
import nextflow.cloud.CloudTransferOptions
import nextflow.exception.AbortOperationException
Expand All @@ -45,7 +46,7 @@ class GoogleLifeSciencesConfig implements CloudTransferOptions {

public final static String DEFAULT_SSH_IMAGE = 'gcr.io/cloud-genomics-pipelines/tools'

public final static String DEFAULT_ENTRY_POINT = '/bin/bash'
public final static String DEFAULT_ENTRY_POINT = NF.bash()

public final static int DEF_PARALLEL_THREAD_COUNT =1
public final static int DEF_DOWNLOAD_MAX_COMPONENTS =8
Expand Down
5 changes: 5 additions & 0 deletions plugins/nf-google/src/test/nextflow/cloud/google/batch/GoogleBatchExecutorTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ package nextflow.cloud.google.batch
import java.nio.file.Path

import com.google.cloud.storage.contrib.nio.CloudStorageFileSystem
import nextflow.Global
import nextflow.Session
import nextflow.SysEnv
import nextflow.processor.TaskHandler
Expand All @@ -22,6 +23,10 @@ import spock.lang.Unroll
*/
class GoogleBatchExecutorTest extends Specification {

def setup() {
Global.session = Mock(Session) { getConfig()>>[:] }
}

def 'should check is fusion' () {
given:
SysEnv.push(ENV)
Expand Down
3 changes: 2 additions & 1 deletion plugins/nf-google/src/test/nextflow/cloud/google/batch/logging/BatchLoggingTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.google.cloud.logging.LogEntry
import com.google.cloud.logging.Payload.StringPayload
import com.google.cloud.logging.Severity
import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.Session
import nextflow.cloud.google.batch.client.BatchClient
import nextflow.cloud.google.batch.client.BatchConfig
Expand Down Expand Up @@ -99,7 +100,7 @@ class BatchLoggingTest extends Specification {

when:
def imageUri = 'quay.io/nextflow/bash'
def cmd = ['/bin/bash','-c','echo "Hello world!" && echo "Oops something went wrong" >&2']
def cmd = [NF.bash(), '-c', 'echo "Hello world!" && echo "Oops something went wrong" >&2']
def req = Job.newBuilder()
.addTaskGroups(
TaskGroup.newBuilder()
Expand Down
Loading