Make Bash path configurable #5696
Conversation
Signed-off-by: Paolo Di Tommaso <[email protected]>
✅ Deploy Preview for nextflow-docs-staging canceled.
|
|
|
||
| @Memoized | ||
| static String bash() { | ||
| session().config.navigate('nextflow.defaults.bash') ?: '/bin/bash' |
There was a problem hiding this comment.
This config scope was renamed to workflow, I would just call it workflow.bash
There was a problem hiding this comment.
let's discuss about this. I think we need to nextflow wide setting (more tied to runtime than workflow concept)
| BASH = Collections.unmodifiableList( level > 0 ? ['/bin/bash','-uex'] : ['/bin/bash','-ue'] ) | ||
|
|
||
| BASH = Collections.unmodifiableList( level > 0 ? [NF.bash(), '-uex'] : [NF.bash(), '-ue'] ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| @@ -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'] ) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Got it. What's preventing creating a symlink to "normalise" it?
There was a problem hiding this comment.
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.
Signed-off-by: Paolo Di Tommaso <[email protected]>
|
Thank you for addressing my issues @pditommaso. NixOS currently patches Nextflow so that it can run there even though Now, #5684 got also closed and my approach will not be merged (reasoning is mentioned here) in favor of this PR here. I believe this PR here does not improve the situation for NixOS (see my reasoning here and here). Thus, I see two options now
I can test option 1) but would need advice how you envision this to be used. However, we might also directly agree on option 2). (For option 2, it would make my life easier if this PR here does not get merged. But this is of course entirely up to others to decide) |
This PR allows the customisation of Bash interpreter used by Nextflow go a user provided path defined via nextflow config file or an environment variable (to be done).