common-workflow-language / cwltool

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

[windows] if the Windows default container is used, print a warning #469

Closed mr-c closed 7 years ago

mr-c commented 7 years ago

But if the default container wasn't needed then don't mention it.

Warning text (may need to be refined):

"""We are on Microsoft Windows and not all components of this CWL description have a
container specified. The default container is %s.

Note, this could affect portability if this CWL description relies on non-POSIX features
or commands in this container. For best results add the following to your CWL
description's hints section:

hints:
  DockerRequirement:
    dockerPull: %s
""" % windows_default_container, windows_default_container
mr-c commented 7 years ago

Comments on the wording are appreciated, especially from @jvdzwaan

kapilkd13 commented 7 years ago

Hi Michael, Can you explain if default container isn't used don't show this warning. But on windows all execution is happening under docker container, so I guess default container is always used, isn't it. Do you mean if external Container is already provided.

jvdzwaan commented 7 years ago

Good idea to add a warning! I would make it more explicit that all steps without a docker hint/requirement will be executed in the default container. Something like:

We are on Microsoft Windows and not all components of this CWL description have a
container specified. This means that these steps will be executed in the default container, 
which is %s.

Also, I don't understand why you would advise to add a hint for the default container. Shouldn't we advise them to use a more appropriate container if they have non-POSIX features in their CWL?

mr-c commented 7 years ago

@kapilkd13 correct, if all components of the Workflow, CommandLineTool, or ExpressionTool have a DockerRequirement or inherit a DockerRequirement (except from the --default-container or the Windows fallback default container) then the warning should be printed under Windows.

mr-c commented 7 years ago

@jvdzwaan The default container used by cwltool could change over time. Your idea that there should be a note suggesting that (if needed) the user should specify another container, is a good idea.

mr-c commented 7 years ago

Fixed in #492