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

Replace /bin/bash with /usr/bin/env bash #2341

Closed
wants to merge 1 commit into from

Conversation

unode
Copy link

@unode unode commented Sep 22, 2021

This replaces all hardcoded /bin/bash paths to /usr/bin/env bash.

When additional arguments are given to bash (e.g. bash -ue), /usr/bin/env -S bash -ue is used instead.
The env -S functionality requires coreutils 8.30 or newer. This or a newer version of coreutils is present since Ubuntu 19.04, CentOS 8 and many others.

This PR also addresses issues with containers and Linux OSes where /bin/bash doesn't exist or where bash is installed in a different path (e.g. NixOS).
The PR should also fix partially or entirely the issues #73 #1598 and #1732

Additional potential issues remain in the files:

modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy
modules/nextflow/src/main/groovy/nextflow/k8s/K8sDriverLauncher.groovy
modules/nextflow/src/test/groovy/nextflow/container/DockerBuilderTest.groovy
modules/nextflow/src/test/groovy/nextflow/container/PodmanBuilderTest.groovy
modules/nextflow/src/test/groovy/nextflow/container/ShifterBuilderTest.groovy
modules/nextflow/src/test/groovy/nextflow/container/UdockerBuilderTest.groovy
modules/nextflow/src/test/groovy/nextflow/executor/BashWrapperBuilderTest.groovy

Where /bin/bash is still hardcoded to work with different images.
In fact, some of that code is likely non-functional as, for instance, the latest busybox docker image no longer includes /bin/bash.

Where 'bash -ue' is used, the replacement reads '/usr/bin/env -S bash -ue'
and requires coreutils 8.30 or newer (ubuntu:19.04 or newer)

Signed-off-by: Renato Alves <[email protected]>
@pditommaso
Copy link
Member

The PR should also fix partially or entirely the issues #73 #1598 and #1732

Sorry, can't merge this. This issue was already discussed #1598. The use of /bin/bash is a fair assumption. If the host uses a different path for the bash binary, it could be easily fixed by adding a symlink.

@pditommaso pditommaso closed this Sep 23, 2021
@unode
Copy link
Author

unode commented Sep 23, 2021

As explained in the description, that's not quite true and there are many cases where this may fail, NixOS being only one. Others include systems where /bin/bash is too old or incompatible (e.g. when LD_LIBRARY_PATH is tampered with due to complicated software stacks), in which case a newer bash may live somewhere in the user's $PATH.

As for a symlink, you seem to be assuming that users will have control over /bin/bash. This is rarely the case.

The proposed PR addresses all these.

One could make a case for env -S but if that's a concern, can we discuss alternatives, such as converting all the hardcoded shebangs and identical bash invocations into a user defined path, perhaps as a command-line argument?

TomYipOracle added a commit to TomYipOracle/ncov2019-artic-nf that referenced this pull request Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants