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

Argument with prefix = empty string and separate = false is omitted from cmd line #1165

Open
bogdang989 opened this issue Aug 9, 2019 · 3 comments

Comments

@bogdang989
Copy link
Member

bogdang989 commented Aug 9, 2019

Expected Behavior

Argument with prefix: "" and separate: false should be in the command line.
Command line should be echo non_existing_app.

Actual Behavior

Argument is not in the command line.
Command line is non_existing_app.

Workflow Code

class: CommandLineTool
cwlVersion: v1.0
baseCommand: []
inputs:
  echo: boolean
outputs: []
arguments:
- position: 1
  shellQuote: false
  prefix: ''
  valueFrom: "non_existing_app"
- position: 0
  prefix: ''
  separate: false
  shellQuote: false
  valueFrom: "echo"
requirements:
- class: ShellCommandRequirement

Your Environment

  • cwltool version: 1.0.20190621234233
    works fine with cwltool version: 1.0.20190228155703
@sivkovic
Copy link

I believe this caused the error 239499d#diff-bfbf952b5c2585de423b12dcf6f43938R424. Is this the proper behavior if we don't include the prefix that value should not be added? Either that or there should probably be additional clarification in the spec (https://www.commonwl.org/v1.1/CommandLineTool.html#CommandLineBinding)

@bogdang989
Copy link
Member Author

bogdang989 commented Aug 16, 2019

Hi @sivkovic 👋 It's definitely a bug, value shouldn't be excluded if prefix is empty string, nothing in the spec indicates that.
Here's a PR for the cwltool fix (basically just to revert the check from if not prefix to if prefix is None) - #1166 and a PR for a conformance test that covers this case for v1.1 spec - common-workflow-language/cwl-v1.1#64

EDIT: Actually, when separate is false, prefix must be defined or validation would fail. Therefore, there is no need to even check for prefix at all, just append should work.

@mr-c
Copy link
Member

mr-c commented Oct 31, 2020

Since this is a change in behaviour, I guess it would have to wait for CWL v1.3 or 2.0...

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

No branches or pull requests

3 participants