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

Template: Fix process.shell in nextflow.config #3416

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

ewels
Copy link
Member

@ewels ewels commented Jan 21, 2025

The latest Nextflow edge release included some changes in how the bash shell command is used. This surfaced a problem that was introduced into the pipeline template in #2991 - the process.shell directive should be either a single-line or a list, not a multi-line string (as far as I can tell, there was no way for us to know this).

Reverting this to go back to using a list as before, which solves the problem found in nextflow-io/nextflow#5686

See also nextflow-io/nextflow#5689 which strengthens Nextflow's validation logic for this directive. Once that is merged and released, pipelines with the multi-line string above will not run and instead fail with an error.

@ewels ewels added the template nf-core pipeline/component template label Jan 21, 2025
@ewels ewels added high-priority bug Something isn't working labels Jan 21, 2025
@mashehu
Copy link
Contributor

mashehu commented Jan 21, 2025

@nf-core-bot changelog

@mashehu mashehu changed the title Fix process.shell in nextflow.config Template: Fix process.shell in nextflow.config Jan 21, 2025
@ewels ewels enabled auto-merge January 21, 2025 14:01
@ewels ewels merged commit 0b6b151 into nf-core:dev Jan 21, 2025
88 checks passed
@mahesh-panchal
Copy link
Member

This isn't working.
It's not preventing file clobbering

@mirpedrol
Copy link
Member

This isn't working.
It's not preventing file clobbering

We still have the -C option https://github.com/nf-core/tools/pull/3416/files#diff-957ebc7549e67b6aa1df19ae402551fee3e2e06afbc7c563424a4515b1434627R243. Is this not working?

@mahesh-panchal
Copy link
Member

No. It produces the header:

#!/usr/bin/env bash -C -e -u -o pipefail

which doesn't work. Anything after bash is ignored by env.

@ewels ewels deleted the fix-process-shell branch January 21, 2025 15:10
JoseEspinosa added a commit to JoseEspinosa/nf-core-proteinfold that referenced this pull request Jan 21, 2025
mahesh-panchal added a commit to mahesh-panchal/nf-core-tools that referenced this pull request Jan 21, 2025
@ewels
Copy link
Member Author

ewels commented Jan 21, 2025

Interesting. Is this because it's bash and not /bin/bash? That was the other change in #2991

This is what we had for many years, which I think worked?

process.shell = ['/bin/bash', '-euo', 'pipefail']

@mahesh-panchal
Copy link
Member

Yes, exactly. /bin/bash is calling the bash interpreter directly. This PR is calling env, not bash.

@jorgee
Copy link

jorgee commented Jan 21, 2025

Yes, exactly. /bin/bash is calling the bash interpreter directly. This PR is calling env, not bash.

Where is env called? I just see the changes from set to options.

@ewels
Copy link
Member Author

ewels commented Jan 21, 2025

Ok slowly working my way through what's happened here. I think:

  • Forever ago
    • process.shell = ['/bin/bash', '-euo', 'pipefail']
    • Works but locks /bin/bash so bad for nix and places that don't have this
  • Last year's template change from @mahesh-panchal
    • process.shell multi-line string
    • Uses bash which Nextflow resolves to /usr/bin/env bash so works on nix and others
    • Bash flags are separate commands with set
    • Works everywhere
  • Nextflow edge release
    • Doesn't support multi-line strings, so crashes because of .command.run
  • Fix above
    • Fixes .command.run by removing multi-line string, but doesn't work with flags because of /usr/bin/env bash

@jorgee has a nice solution to add the -S flag for env:

-S/--split-string usage in scripts
       The -S option allows specifying multiple parameters in a script.

This then means that the script only works with env, so we should probably fix that too. Which leads me to:

// Set bash options
process.shell = [
    "/usr/bin/env",
    "-S",         // Allows split string for /usr/bin/env
    "bash",
    "-C",         // No clobber - prevent output redirection from overwriting files.
    "-e",         // Exit if a tool returns a non-zero status/exit code
    "-u",         // Treat unset variables and parameters as an error
    "-o pipefail" // Returns the status of the last command to exit with a non-zero status or zero if all successfully execute
]

Thoughts?

@mahesh-panchal
Copy link
Member

mahesh-panchal commented Jan 21, 2025

I tried -S and it didn't work (in Gitpod nf-core container), so I resorted to what's in #3417

@ewels
Copy link
Member Author

ewels commented Jan 22, 2025

See also nextflow-io/nextflow#5684

fbdtemme added a commit to nf-core/pixelator that referenced this pull request Jan 22, 2025
@pditommaso
Copy link

I think it should be:

process.shell = [
    "bash",
    "-C",         // No clobber - prevent output redirection from overwriting files.
    "-e",         // Exit if a tool returns a non-zero status/exit code
    "-u",         // Treat unset variables and parameters as an error
    "-o",         
    "pipefail"    // Returns the status of the last command to exit with a non-zero status or zero if all successfully execute
]

heuermh added a commit to heuermh/nf-core-pangenome that referenced this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high-priority template nf-core pipeline/component template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants