common-workflow-language / cwltool

Common Workflow Language reference implementation
https://cwltool.readthedocs.io/
Apache License 2.0
335 stars 230 forks source link

cwltool expects string for envDef #1148

Open gijzelaerr opened 5 years ago

gijzelaerr commented 5 years ago

this works:

cwlVersion: v1.0
class: CommandLineTool
baseCommand: env
requirements:
  EnvVarRequirement:
    envDef:
      HELLO: hi
inputs: []
outputs: []

but if i change it to

cwlVersion: v1.0
class: CommandLineTool
baseCommand: env
requirements:
  EnvVarRequirement:
    envDef:
      HELLO: 10
inputs: []
outputs: []

it breaks:

λ  .venv3/bin/cwltool test.cwl
.venv3/bin/cwltool 1.0.20181217162649
Resolved 'test.cwl' to 'file:///home/gijs/Work/apercal/test.cwl'
Tool definition failed validation:
test.cwl:1:1: Object `test.cwl` is not valid because
                tried `CommandLineTool` but
test.cwl:4:1:     the `requirements` field is not valid because
                    tried array of <InlineJavascriptRequirement or SchemaDefRequirement or
                    DockerRequirement or SoftwareRequirement or InitialWorkDirRequirement or
                    EnvVarRequirement or ShellCommandRequirement or ResourceRequirement or
                    SubworkflowFeatureRequirement or ScatterFeatureRequirement or
                    MultipleInputFeatureRequirement or StepInputExpressionRequirement> but
test.cwl:5:3:         item is invalid because
test.cwl:6:5:           the `envDef` field is not valid because
test.cwl:7:7:             item is invalid because
                            the `envValue` field is not valid because
                              - tried string but
                                  the value is not string
                              - tried Expression but
                                  value is a int but expected a string

It took me a while to figure it out, but 10 is automatically converted to a int type, which envDev doesn't like. Escaping the integer solves the problem, but is counterintuitive. Probably better to let envDef handle ints as well (automatically convert it to a string).

mr-c commented 5 years ago

This could be fixed by updating the v1.x schema. @tetron what do you think?

tetron commented 5 years ago

I feel like it would be better to tweak schema salad to give a better error message telling you that you have a number where it expects a string and you need to quote it.

The problem with coercing numbers to strings is there's all sorts of ways that something that was parsed as a number gets printed back as a different string that isn't character-for-character identical than what was in the original file. Then you're even more confused.