Refactor secondaryFile handling to avoid basing on primary "location".

Instead prefer to use basename or path.  Necessary to support engines such as
Toil with non-filename-preserving storage systems.

Author: Peter Amstutz

cwltool/builder.py

@@ -1,6 +1,7 @@
 from __future__ import absolute_import
 import copy
 import os
+import logging
 from typing import Any, Callable, Dict, List, Text, Type, Union
 
 import six
@@ -18,6 +19,8 @@
 from .stdfsaccess import StdFsAccess
 from .utils import aslist, get_feature, docker_windows_path_adjust, onWindows
 
+_logger = logging.getLogger("cwltool")
+
 AvroSchemaFromJSONData = avro.schema.make_avsc_object
 
 CONTENT_LIMIT = 64 * 1024
@@ -145,18 +148,25 @@ def bind_input(self, schema, datum, lead_pos=None, tail_pos=None):
                         datum["secondaryFiles"] = []
                     for sf in aslist(schema["secondaryFiles"]):
                         if isinstance(sf, dict) or "$(" in sf or "${" in sf:
-                            secondary_eval = self.do_eval(sf, context=datum)
-                            if isinstance(secondary_eval, string_types):
-                                sfpath = {"location": secondary_eval,
-                                          "class": "File"}
-                            else:
-                                sfpath = secondary_eval
-                        else:
-                            sfpath = {"location": substitute(datum["location"], sf), "class": "File"}
-                        if isinstance(sfpath, list):
-                            datum["secondaryFiles"].extend(sfpath)
+                            sfpath = self.do_eval(sf, context=datum)
                         else:
-                            datum["secondaryFiles"].append(sfpath)
+                            sfpath = substitute(datum["basename"], sf)
+                        for sfname in aslist(sfpath):
+                            found = False
+                            for d in datum["secondaryFiles"]:
+                                if not d.get("basename"):
+                                    d["basename"] = d["location"][d["location"].rindex("/")+1:]
+                                if d["basename"] == sfname:
+                                    found = True
+                            if not found:
+                                if isinstance(sfname, dict):
+                                    datum["secondaryFiles"].append(sfname)
+                                else:
+                                    datum["secondaryFiles"].append({
+                                        "location": datum["location"][0:datum["location"].rindex("/")+1]+sfname,
+                                        "basename": sfname,
+                                        "class": "File"})
+
                     normalizeFilesDirs(datum["secondaryFiles"])
 
                 def _capture_files(f):

cwltool/draft2tool.py

@@ -35,7 +35,7 @@
 ACCEPTLIST_EN_RELAXED_RE = re.compile(r".*")  # Accept anything
 ACCEPTLIST_RE = ACCEPTLIST_EN_STRICT_RE
 DEFAULT_CONTAINER_MSG="""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, 
+container specified. This means that these steps will be executed in the default container,
 which is %s.
 
 Note, this could affect portability if this CWL description relies on non-POSIX features
@@ -116,17 +116,26 @@ def revmap_file(builder, outdir, f):
     if not split.scheme:
         outdir = file_uri(str(outdir))
 
+    # builder.outdir is the inner (container/compute node) output directory
+    # outdir is the outer (host/storage system) output directory
+
     if "location" in f:
         if f["location"].startswith("file://"):
             path = convert_pathsep_to_unix(uri_file_path(f["location"]))
             revmap_f = builder.pathmapper.reversemap(path)
+
             if revmap_f and not builder.pathmapper.mapper(revmap_f[0]).type.startswith("Writable"):
                 f["basename"] = os.path.basename(path)
-                f["location"] = revmap_f[0]
+                f["location"] = revmap_f[1]
             elif path == builder.outdir:
                 f["location"] = outdir
             elif path.startswith(builder.outdir):
                 f["location"] = builder.fs_access.join(outdir, path[len(builder.outdir) + 1:])
+            elif f["location"].startswith(outdir):
+                revmap_f = builder.pathmapper.reversemap(builder.fs_access.join(builder.outdir, f["location"][len(outdir) + 1:]))
+                if revmap_f and not builder.pathmapper.mapper(revmap_f[0]).type.startswith("Writable"):
+                    f["basename"] = os.path.basename(path)
+                    f["location"] = revmap_f[1]
         return f
 
     if "path" in f:
@@ -350,6 +359,7 @@ def rm_pending_output_callback(output_callbacks, jobcachepending,
         if "stagedir" in make_path_mapper_kwargs:
             make_path_mapper_kwargs = make_path_mapper_kwargs.copy()
             del make_path_mapper_kwargs["stagedir"]
+
         builder.pathmapper = self.makePathMapper(reffiles, builder.stagedir, **make_path_mapper_kwargs)
         builder.requirements = j.requirements
 
@@ -566,7 +576,10 @@ def collect_output(self, schema, builder, outdir, fs_access, compute_checksum=Tr
                         elif gb.startswith("/"):
                             raise WorkflowException("glob patterns must not start with '/'")
                         try:
+                            prefix = fs_access.glob(outdir)
                             r.extend([{"location": g,
+                                       "path": fs_access.join(builder.outdir, g[len(prefix[0])+1:]),
+                                       "basename": os.path.basename(g),
                                        "class": "File" if fs_access.isfile(g) else "Directory"}
                                       for g in fs_access.glob(fs_access.join(outdir, gb))])
                         except (OSError, IOError) as e:
@@ -576,12 +589,14 @@ def collect_output(self, schema, builder, outdir, fs_access, compute_checksum=Tr
                             raise
 
                 for files in r:
+                    rfile = files.copy()
+                    revmap(rfile)
                     if files["class"] == "Directory":
                         ll = builder.loadListing or (binding and binding.get("loadListing"))
                         if ll and ll != "no_listing":
                             get_listing(fs_access, files, (ll == "deep_listing"))
                     else:
-                        with fs_access.open(files["location"], "rb") as f:
+                        with fs_access.open(rfile["location"], "rb") as f:
                             contents = b""
                             if binding.get("loadContents") or compute_checksum:
                                 contents = f.read(CONTENT_LIMIT)
@@ -625,28 +640,29 @@ def collect_output(self, schema, builder, outdir, fs_access, compute_checksum=Tr
                     else:
                         r = r[0]
 
-            # Ensure files point to local references outside of the run environment
-            adjustFileObjs(r, cast(  # known bug in mypy
-                # https://github.com/python/mypy/issues/797
-                Callable[[Any], Any], revmap))
-
             if "secondaryFiles" in schema:
                 with SourceLine(schema, "secondaryFiles", WorkflowException):
                     for primary in aslist(r):
                         if isinstance(primary, dict):
-                            primary["secondaryFiles"] = []
+                            primary.setdefault("secondaryFiles", [])
                             for sf in aslist(schema["secondaryFiles"]):
                                 if isinstance(sf, dict) or "$(" in sf or "${" in sf:
                                     sfpath = builder.do_eval(sf, context=primary)
                                     if isinstance(sfpath, string_types):
-                                        sfpath = revmap({"location": sfpath, "class": "File"})
+                                        sfpath = {"path": primary["path"][0:primary["path"].rindex("/")+1]+sfpath, "class": "File"}
                                 else:
-                                    sfpath = {"location": substitute(primary["location"], sf), "class": "File"}
-
+                                    sfpath = {"path": substitute(primary["path"], sf), "class": "File"}
                                 for sfitem in aslist(sfpath):
+                                    if "path" in sfitem and "location" not in sfitem:
+                                        revmap(sfitem)
                                     if fs_access.exists(sfitem["location"]):
                                         primary["secondaryFiles"].append(sfitem)
 
+            # Ensure files point to local references outside of the run environment
+            adjustFileObjs(r, cast(  # known bug in mypy
+                # https://github.com/python/mypy/issues/797
+                Callable[[Any], Any], revmap))
+
             if not r and optional:
                 r = None