Move var/spool/cwl detector to process.py and make it a warning. (#733)

Move var/spool/cwl detector to process.py and make it a warning.

Do the check alongside other static checks.

Author: tetron

cwltool/load_tool.py

@@ -224,33 +224,6 @@ def validate_document(document_loader,  # type: Loader
 
         workflowobj = fetch_document(uri, fetcher_constructor=fetcher_constructor)[1]
 
-    def var_spool_cwl_detector(obj,           # type: Union[Mapping, Iterable, Text]
-                               item=None,     # type: Optional[Any]
-                               obj_key=None,  # type: Optional[Any]
-                              ):              # type: (...)->None
-        """ Detects any textual reference to /var/spool/cwl. """
-        if isinstance(obj, string_types):
-            if "var/spool/cwl" in obj:
-                message = SourceLine(
-                    item=item, key=obj_key, raise_type=Text,
-                    include_traceback=_logger.isEnabledFor(logging.DEBUG)).makeError(
-                        "Non-portable reference to /var/spool/cwl found: "
-                        "'{}'.\n Replace with /var/spool/cwl/ with "
-                        "$(runtime.outdir).".format(obj))
-                if not strict:
-                    _logger.warning(message)
-                else:
-                    raise ValidationException(message)
-            else:
-                return
-        elif isinstance(obj, Mapping):
-            for key, value in iteritems(obj):
-                var_spool_cwl_detector(value, obj, key)
-        elif isinstance(obj, Iterable):
-            for element in obj:
-                var_spool_cwl_detector(element, obj, None)
-    var_spool_cwl_detector(workflowobj)
-
     fileuri = urllib.parse.urldefrag(uri)[0]
     if "cwlVersion" not in workflowobj:
         if metadata and 'cwlVersion' in metadata:

cwltool/process.py

@@ -16,7 +16,7 @@
 from io import open
 from functools import cmp_to_key
 from typing import (Any, Callable, Dict, Generator, List, Set, Text,
-                    Tuple, Union, cast)
+                    Tuple, Union, cast, Optional)
 
 import schema_salad.schema as schema
 import schema_salad.validate as validate
@@ -28,6 +28,7 @@
 from schema_salad.ref_resolver import Loader, file_uri
 from schema_salad.sourceline import SourceLine
 from six.moves import urllib
+from six import iteritems, itervalues, string_types
 
 from .validate_js import validate_js_expressions
 from .utils import cmp_like_py2
@@ -388,6 +389,32 @@ def get_overrides(overrides, toolid):  # type: (List[Dict[Text, Any]], Text) ->
             req.update(ov)
     return req
 
+
+def var_spool_cwl_detector(obj,           # type: Union[Dict, List, Text]
+                           item=None,     # type: Optional[Any]
+                           obj_key=None,  # type: Optional[Any]
+                          ):              # type: (...)->bool
+    """ Detects any textual reference to /var/spool/cwl. """
+    r = False
+    if isinstance(obj, string_types):
+        if "var/spool/cwl" in obj and obj_key != "dockerOutputDirectory":
+            _logger.warn(SourceLine(
+                item=item, key=obj_key, raise_type=Text).makeError(
+"""Non-portable reference to /var/spool/cwl detected:
+  '{}'
+To fix, replace /var/spool/cwl with $(runtime.outdir) or
+  add DockerRequirement to the 'requirements' section and
+  declare 'dockerOutputDirectory: /var/spool/cwl'.""".format(obj)))
+            r = True
+    elif isinstance(obj, dict):
+        for key, value in iteritems(obj):
+            r = var_spool_cwl_detector(value, obj, key) or r
+    elif isinstance(obj, list):
+        for key, value in enumerate(obj):
+            r = var_spool_cwl_detector(value, obj, key) or r
+    return r
+
+
 class Process(six.with_metaclass(abc.ABCMeta, object)):
     def __init__(self, toolpath_object, **kwargs):
         # type: (Dict[Text, Any], **Any) -> None
@@ -499,6 +526,24 @@ def __init__(self, toolpath_object, **kwargs):
 
             validate_js_expressions(cast(CommentedMap, toolpath_object), self.doc_schema.names[toolpath_object["class"]], validate_js_options)
 
+        dockerReq, is_req = self.get_requirement("DockerRequirement")
+
+        if dockerReq and dockerReq.get("dockerOutputDirectory") and not is_req:
+            _logger.warn(SourceLine(
+                item=dockerReq, raise_type=Text).makeError(
+"""When 'dockerOutputDirectory' is declared, DockerRequirement
+  should go in the 'requirements' section, not 'hints'."""))
+
+        if dockerReq and dockerReq.get("dockerOutputDirectory") == "/var/spool/cwl":
+            if is_req:
+                # In this specific case, it is legal to have /var/spool/cwl, so skip the check.
+                pass
+            else:
+                # Must be a requirement
+                var_spool_cwl_detector(self.tool)
+        else:
+            var_spool_cwl_detector(self.tool)
+
     def _init_job(self, joborder, **kwargs):
         # type: (Dict[Text, Text], **Any) -> Builder
         """

tests/non_portable2.cwl

@@ -0,0 +1,24 @@
+#!/usr/bin/env cwl-runner
+cwlVersion: v1.0
+class: CommandLineTool
+
+hints:
+  DockerRequirement:
+    dockerPull: debian
+    dockerOutputDirectory: /var/spool/cwl
+  InitialWorkDirRequirement:
+    listing:
+      - class: File
+        basename: hi.txt
+        contents: Hello, World!
+
+inputs: []
+
+baseCommand:
+ - cat
+ - /var/spool/cwl/hi.txt
+
+stdout: result.txt
+
+outputs:
+  result: stdout

tests/portable.cwl

@@ -0,0 +1,24 @@
+#!/usr/bin/env cwl-runner
+cwlVersion: v1.0
+class: CommandLineTool
+
+requirements:
+  DockerRequirement:
+    dockerPull: debian
+    dockerOutputDirectory: /var/spool/cwl
+  InitialWorkDirRequirement:
+    listing:
+      - class: File
+        basename: hi.txt
+        contents: Hello, World!
+
+inputs: []
+
+baseCommand:
+ - cat
+ - /var/spool/cwl/hi.txt
+
+stdout: result.txt
+
+outputs:
+  result: stdout

tests/test_examples.py

@@ -4,6 +4,7 @@
 import subprocess
 from os import path
 import sys
+import logging
 
 from io import StringIO
 
@@ -556,13 +557,49 @@ def test_checker(self):
             f = cwltool.factory.Factory()
             f.make(get_data("tests/checker_wf/broken-wf2.cwl"))
 
-    def test_var_spool_cwl_checker(self):
+    def test_var_spool_cwl_checker1(self):
         """ Confirm that references to /var/spool/cwl are caught."""
-        with self.assertRaises(schema_salad.validate.ValidationException):
+
+        stream = StringIO()
+        streamhandler = logging.StreamHandler(stream)
+        _logger = logging.getLogger("cwltool")
+        _logger.addHandler(streamhandler)
+
+        try:
             f = cwltool.factory.Factory()
             f.make(get_data("tests/non_portable.cwl"))
-        f = cwltool.factory.Factory(strict=False)
-        f.make(get_data("tests/non_portable.cwl"))
+            self.assertIn("non_portable.cwl:18:4: Non-portable reference to /var/spool/cwl detected", stream.getvalue())
+        finally:
+            _logger.removeHandler(streamhandler)
+
+    def test_var_spool_cwl_checker2(self):
+        """ Confirm that references to /var/spool/cwl are caught."""
+
+        stream = StringIO()
+        streamhandler = logging.StreamHandler(stream)
+        _logger = logging.getLogger("cwltool")
+        _logger.addHandler(streamhandler)
+
+        try:
+            f = cwltool.factory.Factory()
+            f.make(get_data("tests/non_portable2.cwl"))
+            self.assertIn("non_portable2.cwl:19:4: Non-portable reference to /var/spool/cwl detected", stream.getvalue())
+        finally:
+            _logger.removeHandler(streamhandler)
+
+    def test_var_spool_cwl_checker3(self):
+        """ Confirm that references to /var/spool/cwl are caught."""
+
+        stream = StringIO()
+        streamhandler = logging.StreamHandler(stream)
+        _logger = logging.getLogger("cwltool")
+        _logger.addHandler(streamhandler)
+        try:
+            f = cwltool.factory.Factory()
+            f.make(get_data("tests/portable.cwl"))
+            self.assertNotIn("Non-portable reference to /var/spool/cwl detected", stream.getvalue())
+        finally:
+            _logger.removeHandler(streamhandler)
 
 class TestPrintDot(unittest.TestCase):
     def test_print_dot(self):