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

Add newlines and set command to fix #3416 #3417

Closed
wants to merge 2 commits into from

Conversation

mahesh-panchal
Copy link
Member

Fixes the fix to template #3416.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@mahesh-panchal mahesh-panchal requested a review from ewels January 21, 2025 15:13
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Please don't merge this until we confirm that it isn't a problem on the Nextflow side.

I think that process.shell is used in more places than just the header of .command.sh which is how we got here in the first place, as it broke in .command.run. So multi-line commands can be problematic.

@ewels
Copy link
Member

ewels commented Jan 21, 2025

Yeah, I think today's Nextflow PR will trigger a failure if we merge this PR. See here:

if( !it || it.contains('\n') || it.contains('\r') )
    throw new IllegalArgumentException("Directive `process.shell` cannot contain new-line characters - offending value: ${shell}")

@mahesh-panchal
Copy link
Member Author

The alternative is to modify the modules template to set these flags and a mass nf-core modules update to insert the flags. And then remove the flags from the shebang.

@ewels
Copy link
Member

ewels commented Jan 22, 2025

I think that there should be a better way. I think even setting process.shell back to /usr/bash would be preferable tbh.

Lots in play at the moment, see also nextflow-io/nextflow#5684

@mahesh-panchal
Copy link
Member Author

mahesh-panchal commented Jan 24, 2025

Closing as the old method actually works, and I made a mistake in testing.

@mahesh-panchal mahesh-panchal deleted the fix_template_fix branch January 24, 2025 12:35
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.

4 participants