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

rollf · 2024-10-25T10:07:47Z

UPDATE: After having created this PR I realized that I could do better (see comments below) and have also invested more time to understand the code base. Now, I (hopefully) have a version that fits well and is satisfying for everybody. Please refer to all the comments below before looking into the diff.

ORIGINAL PR description:
(This comment was made when 29df9b7 was HEAD.)

This PR allows to use nextfow on NixOS where /bin/bash does not exist. This is achieved by invoking /usr/bin/env -S bash where necessary. This approach (or similar) has been discussed previously:

However, compared the previous discussions/implementations, I specifically do not propose to change /bin/bash unless really necessary. By "necessary" I mean that bash is invoked on a local host system where it may not be available. Namely, when <task>/.command.run shall be invoked (always on the host) and when <task>/.command.sh is invoked in a non-container context (i.e. also on the host). I have not touched any other occurrences of /bin/bash, esp. not within the container.

My initial implementation used which bash instead if /usr/bin/env -S bash but I esteemed the latter to be more correct. Please refer to the git commit history/messages for some further insights.

I understand that it is possible for downstream consumers to work around /bin/bash (symlink was proposed in one of the linked issues above). However, I believe that my proposal/approach (=assume /bin/bash to be available everywhere except on the local host where nextflow is invoked) here is not too intrusive.

I'm happy to change the the code as requested. I have not added any comments since I expect (at least) change requests. Please review this with an unbiased mind set. Multiple people have expressed their intention to use nextflow on systems without /bin/bash.

I have tested solely on NixOS.

Further context:

Here is the corresponding nixpkgs issue that motivates this PR.

Here is the current package definition for Nextflow in NixOS which causes trouble (see nixpkgs issue above).

Here is a new package definition assuming Nextflow would be compatible with NixOS out of the box.

On systems without /bin/bash (e.g. NixOS), invoking tasks may fail because of some hard-coded references to /bin/bash. Those references are either fine (because the task runs in a container where /bin/bash *is* available), don't matter (because the shebang is ignored) or cause failure (because /bin/bash cannot be invoked). This commit adapts Nextflow for the last scenario. Instead of invoking `/bin/bash`, the actually available bash executable is called. Signed-off-by: Rolf Schröder <[email protected]>

netlify · 2024-10-25T10:08:06Z

✅ Deploy Preview for nextflow-docs-staging canceled.

Name	Link
🔨 Latest commit	`1d9839e`
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/671baf7ec2f4320008c771bd

This is more portable. This commit also makes sure that `-ue[x]` is not used during trace. Signed-off-by: Rolf Schröder <[email protected]>

Signed-off-by: Rolf Schröder <[email protected]>

rollf · 2024-10-25T11:05:57Z

On another thought, iff /usr/bin/env -S bash is more acceptable than I think, one could be more intrusive, i.e. use it "everywhere" (where BASH is used) but make sure it is not used within container contexts. This seems to work for me and here would be another commit based on the changes in this PR (i.e. partially revert/invert them).

Either way, I feel it's always a bit ugly to use /usr/bin/env -S bash on the host and /bin/bash in the container because the way Nextflow is designed currently assumes the same interpreter for the host and the container so one has to "manually" make sure /bin/bash is used in the container if /usr/bin/env -S bash got used by the host (but not for tracing where the initial invocation happens on the host, even for container contexts!). I'm not saying the design is wrong here, I just don't like the invocation of .replace() (see changes).

rollf · 2024-10-25T11:57:11Z

(This comment was made before 1d9839e became HEAD.)

Ah, I realize that I have used (for testing) quay.io/nextflow/bash as a docker image which actually has /usr/bin/env so the test was flawed. Playing around with this image yields problems when tracing (because /usr/bin/env is not available). Good that I kept this PR as a draft then ^^. I'll update when I fixed the issue.

Signed-off-by: Rolf Schröder <[email protected]>

to use. Signed-off-by: Rolf Schröder <[email protected]>

rollf · 2024-10-26T06:17:10Z

modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy

@@ -65,6 +65,8 @@ class BashWrapperBuilder {

    static final public List<String> BASH


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

rollf · 2024-10-26T06:17:55Z

modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy

@@ -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"


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

rollf · 2024-10-26T06:18:36Z

modules/nextflow/src/main/groovy/nextflow/executor/local/LocalTaskHandler.groovy

@@ -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()


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

rollf · 2024-10-26T06:19:50Z

modules/nextflow/src/main/groovy/nextflow/processor/TaskBean.groovy

@@ -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


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).

rollf · 2024-10-26T06:20:26Z

modules/nextflow/src/main/groovy/nextflow/processor/TaskConfig.groovy

@@ -375,10 +375,10 @@ class TaskConfig extends LazyMap implements Cloneable {
        return (List<String>) get('secret')
    }

-    List<String> getShell() {
+    List<String> getShell(boolean isContainerEnabled = true) {


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

rollf · 2024-10-28T13:20:51Z

(This comment was made when 1d9839e was HEAD.)

After having created this PR I realized that I could do better (see commit history) and have also invested more time to understand the code base. Now, I (hopefully) have a version that fits well and is satisfying for everybody. In the end, my implementation is more intrusive that I initially thought but it feels right now. The final approach now is this: Right from the process/task (object) creation, consider whether the process/task will run inside a container. If not, use /usr/bin/env -S bash, otherwise keep /bin/bash. This, plus 2 minor tweaks for the execution of .command.run and for tracing allows to run nextflow on systems without /bin/bash.

I keep the original comments (above) /git history for documentation. I hope it is not too confusing.

Again, I'm happy to change the code as requested, manipulate the git history, etc. I have also added some review comments which include suggestions for improvements.

I have improved the nextflow-tests for NixOS. You may find some more details there, too. I made sure to test with a docker image that only has /bin/bash available. I did not find any quay.io image (I have roughly ~20 locally) that does not provide /usr/bin/env. I am not advocating for using /usr/bin/env inside the container. I mentioned this here so that it is clear why I specifically build a new image for the NixOS tests.

IMAGES=$(docker image ls | grep quay.io | awk -v OFS='' '{ print $1,":",$2 }')
for IMAGE in $IMAGES ; do echo $IMAGE ; docker run $IMAGE /usr/bin/env bash; done

rollf · 2024-10-28T13:30:19Z

@pditommaso you might be the right person to look into this.

Please have a look into this with an unprejudiced mind. I spent a lot of time getting this right. This merged PR proves that using /usr/bin/env bash on the host is acceptable.

I'm sorry that this PR is a little bit of a mess. If you find that overwhelming, I'm happy to clean up everything (i.e. delete all comments, squash git history). I kept it so that you may follow my thought process.

bentsherman · 2024-12-06T13:37:47Z

Collecting my thoughts here for now:

this PR would make the host environment -- i.e. local non-container tasks -- always use /usr/bin/env bash
/usr/bin/env bash is proven to work in the host environment, but not in containers, since env might not be available
Use shell directive with trace command in wrapper script #4292 would make the shell directive apply to all bash commands in the task environment
therefore one solution could be to merge Use shell directive with trace command in wrapper script #4292 and use this config to support NixOS:

process.shell = ['/usr/bin/env', '-S', 'bash']

rollf · 2024-12-09T08:01:43Z

@bentsherman Thanks for looking into this! We have touched the same code so it makes sense to coordinate. I like your suggestion. If #4292 is merged, I'll adapt my changes accordingly (and then, as you suggested, adapt the NixOS package itself or maybe see if any other mean is possible).

EDIT: I'd rather make it work natively instead of adapting the package (i.e. patching). This was the whole point of my PR. I'd like to look into this first (after merging #4292).

ewels · 2024-12-09T15:06:39Z

@pditommaso will look into any additional dependencies on /bin/bash in the Nextflow ecosystems (Wave, Fusion, and others).

Use /usr/bin/env -S bash instead of which bash.

29df9b7

This is more portable. This commit also makes sure that `-ue[x]` is not used during trace. Signed-off-by: Rolf Schröder <[email protected]>

rollf force-pushed the nextflow-bin-bash branch from d62a5dd to 29df9b7 Compare October 25, 2024 10:08

rollf mentioned this pull request Oct 25, 2024

Nextflow: When using docker and enabling tracing, processes fail within the docker containers due to paths to /nix/store/bash NixOS/nixpkgs#350183

Open

Use initial boolean variable instead of a derived one.

94fe6b0

Signed-off-by: Rolf Schröder <[email protected]>

rollf added 2 commits October 25, 2024 14:07

Do not invoke /usr/bin/env -S bash when tracing.

9fdd844

Signed-off-by: Rolf Schröder <[email protected]>

Always consider the (non-)container context when deciding which bash

1d9839e

to use. Signed-off-by: Rolf Schröder <[email protected]>

rollf commented Oct 26, 2024

View reviewed changes

rollf marked this pull request as ready for review October 28, 2024 13:21

bentsherman added the software/bash label Dec 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rollf commented Oct 25, 2024 •

edited

Loading

netlify bot commented Oct 25, 2024 •

edited

Loading

rollf commented Oct 25, 2024 •

edited

Loading

rollf commented Oct 25, 2024 •

edited

Loading

rollf Oct 26, 2024

rollf Oct 26, 2024

rollf Oct 26, 2024

rollf Oct 26, 2024

rollf Oct 26, 2024

rollf commented Oct 28, 2024 •

edited by ewels

Loading

rollf commented Oct 28, 2024

bentsherman commented Dec 6, 2024

rollf commented Dec 9, 2024 •

edited

Loading

ewels commented Dec 9, 2024

		@@ -65,6 +65,8 @@ class BashWrapperBuilder {

		static final public List<String> BASH

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

Are you sure you want to change the base?

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

Conversation

rollf commented Oct 25, 2024 • edited Loading

netlify bot commented Oct 25, 2024 • edited Loading

✅ Deploy Preview for nextflow-docs-staging canceled.

rollf commented Oct 25, 2024 • edited Loading

rollf commented Oct 25, 2024 • edited Loading

rollf Oct 26, 2024

Choose a reason for hiding this comment

rollf Oct 26, 2024

Choose a reason for hiding this comment

rollf Oct 26, 2024

Choose a reason for hiding this comment

rollf Oct 26, 2024

Choose a reason for hiding this comment

rollf Oct 26, 2024

Choose a reason for hiding this comment

rollf commented Oct 28, 2024 • edited by ewels Loading

rollf commented Oct 28, 2024

bentsherman commented Dec 6, 2024

rollf commented Dec 9, 2024 • edited Loading

ewels commented Dec 9, 2024

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

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

rollf commented Oct 25, 2024 •

edited

Loading

netlify bot commented Oct 25, 2024 •

edited

Loading

rollf commented Oct 25, 2024 •

edited

Loading

rollf commented Oct 25, 2024 •

edited

Loading

rollf commented Oct 28, 2024 •

edited by ewels

Loading

rollf commented Dec 9, 2024 •

edited

Loading