Add proper tests for env vars and fix a few resulting issues

Author: rupertnash

cwltool/docker.py

@@ -78,8 +78,6 @@ def _check_docker_machine_path(path: Optional[str]) -> None:
 class DockerCommandLineJob(ContainerCommandLineJob):
     """Runs a CommandLineJob in a sofware container using the Docker engine."""
 
-    CONTAINER_TMPDIR: str = "/tmp"  # nosec
-
     def __init__(
         self,
         builder: Builder,

cwltool/env_to_stdout.py

@@ -0,0 +1,33 @@
+r"""Python script that acts like (GNU coreutils) env -0.
+
+When run as a script, it prints the the environment as
+`(VARNAME=value\0)*`.
+
+Ideally we would just use `env -0`, because python (thanks to PEPs 538
+and 540) will set zero to two environment variables to better handle
+Unicode-locale interactions, however BSD familiy implementations of
+`env` do not all support the `-0` flag so we supply this script that
+produces equivalent output.
+"""
+
+import os
+from typing import Dict
+
+
+def deserialize_env(data: str) -> Dict[str, str]:
+    """Deserialize the output of `env -0` to dictionary."""
+    ans = {}
+    for item in data.strip("\0").split("\0"):
+        key, val = item.split("=", 1)
+        ans[key] = val
+    return ans
+
+
+def main() -> None:
+    """Print the null-separated enviroment to stdout."""
+    for k, v in os.environ.items():
+        print(f"{k}={v}", end="\0")
+
+
+if __name__ == "__main__":
+    main()

cwltool/job.py

@@ -38,7 +38,7 @@
 from schema_salad.utils import json_dump, json_dumps
 from typing_extensions import TYPE_CHECKING
 
-from . import run_job
+from . import env_to_stdout, run_job
 from .builder import Builder, HasReqsHints
 from .context import RuntimeContext
 from .errors import UnsupportedRequirement, WorkflowException
@@ -212,7 +212,18 @@ def _execute(
         runtimeContext: RuntimeContext,
         monitor_function=None,  # type: Optional[Callable[[subprocess.Popen[str]], None]]
     ) -> None:
+        """Execute the tool, either directly or via script.
 
+        Note: we are now at the point where self.environment is
+        ignored. The caller is responsible for correctly splitting that
+        into the runtime and env arguments.
+
+        `runtime` is the list of arguments to put at the start of the
+        command (e.g. docker run).
+
+        `env` is the enviroment to be set for running the resulting
+        command line.
+        """
         scr = self.get_requirement("ShellCommandRequirement")[0]
 
         shouldquote = needs_shell_quoting_re.search
@@ -431,7 +442,7 @@ def _required_env(self) -> Dict[str, str]:
         pass
 
     def _preserve_environment_on_containers_warning(
-        self, varname: Optional[str] = None
+        self, varname: Optional[Iterable[str]] = None
     ) -> None:
         """When running in a container, issue a warning."""
         # By default, don't do anything; ContainerCommandLineJob below
@@ -456,8 +467,10 @@ def prepare_environment(
             self._preserve_environment_on_containers_warning()
             env.update(os.environ)
         elif runtimeContext.preserve_environment:
+            self._preserve_environment_on_containers_warning(
+                runtimeContext.preserve_environment
+            )
             for key in runtimeContext.preserve_environment:
-                self._preserve_environment_on_containers_warning(key)
                 try:
                     env[key] = os.environ[key]
                 except KeyError:
@@ -564,6 +577,8 @@ def _required_env(self) -> Dict[str, str]:
 class ContainerCommandLineJob(JobBase, metaclass=ABCMeta):
     """Commandline job using containers."""
 
+    CONTAINER_TMPDIR: str = "/tmp"  # nosec
+
     @abstractmethod
     def get_from_requirements(
         self,
@@ -616,17 +631,17 @@ def add_writable_directory_volume(
         """Append a writable directory mapping to the runtime option list."""
 
     def _preserve_environment_on_containers_warning(
-        self, varname: Optional[str] = None
+        self, varnames: Optional[Iterable[str]] = None
     ) -> None:
         """When running in a container, issue a warning."""
-        if varname is None:
-            _logger.warning(
-                "You have specified `--preserve-entire-environment` while running a container"
-            )
+        if varnames is None:
+            flags = "--preserve-entire-environment"
         else:
-            _logger.warning(
-                f"You have specified `--preserve-environment={varname}` while running a container"
-            )
+            flags = "--preserve-environment={" + ", ".join(varnames) + "}"
+
+        _logger.warning(
+            f"You have specified `{flags}` while running a container which will override variables set in the container. This may break the container, be non-portable, and/or affect reproducibility."
+        )
 
     def create_file_and_add_volume(
         self,
@@ -713,7 +728,6 @@ def run(
         (docker_req, docker_is_req) = self.get_requirement("DockerRequirement")
         self.prov_obj = runtimeContext.prov_obj
         img_id = None
-        env = cast(MutableMapping[str, str], os.environ)
         user_space_docker_cmd = runtimeContext.user_space_docker_cmd
         if docker_req is not None and user_space_docker_cmd:
             # For user-space docker implementations, a local image name or ID
@@ -807,7 +821,11 @@ def run(
                     )
 
         self._setup(runtimeContext)
+
+        # Copy as don't want to modify our env
+        env = dict(os.environ)
         (runtime, cidfile) = self.create_runtime(env, runtimeContext)
+
         runtime.append(str(img_id))
         monitor_function = None
         if cidfile:
@@ -997,8 +1015,13 @@ def terminate():  # type: () -> None
             job_script = os.path.join(job_dir, "run_job.bash")
             with open(job_script, "wb") as _:
                 _.write(job_script_contents.encode("utf-8"))
+
             job_run = os.path.join(job_dir, "run_job.py")
             shutil.copyfile(run_job.__file__, job_run)
+
+            env_getter = os.path.join(job_dir, "env_to_stdout.py")
+            shutil.copyfile(env_to_stdout.__file__, env_getter)
+
             sproc = subprocess.Popen(  # nosec
                 ["bash", job_script.encode("utf-8")],
                 shell=False,  # nosec

cwltool/run_job.py

@@ -18,14 +18,15 @@ def handle_software_environment(cwl_env: Dict[str, str], script: str) -> Dict[st
         return cwl_env
 
     env = cwl_env.copy()
-    with open("output_environment.bash") as env_file:
-        for line in env_file:
-            key, val = line.split("=", 1)
-            if key in ("_", "PWD", "SHLVL", "TMPDIR", "HOME", "_CWLTOOL"):
-                # Skip some variables that are meaningful to the shell
-                # or set specifically by the CWL runtime environment.
-                continue
-            env[key] = val[:-1]  # remove trailing newline
+    with open("output_environment.dat") as _:
+        data = _.read().strip("\0")
+    for line in data.split("\0"):
+        key, val = line.split("=", 1)
+        if key in ("_", "PWD", "SHLVL", "TMPDIR", "HOME", "_CWLTOOL"):
+            # Skip some variables that are meaningful to the shell or set
+            # specifically by the CWL runtime environment.
+            continue
+        env[key] = val
     return env
 
 

cwltool/singularity.py

@@ -83,8 +83,6 @@ def _normalize_sif_id(string: str) -> str:
 
 
 class SingularityCommandLineJob(ContainerCommandLineJob):
-    CONTAINER_TMPDIR: str = "/tmp"  # nosec
-
     def __init__(
         self,
         builder: Builder,
@@ -277,13 +275,12 @@ def append_volume(
         runtime: List[str], source: str, target: str, writable: bool = False
     ) -> None:
         runtime.append("--bind")
-        runtime.append(
-            "{}:{}:{}".format(
-                source,
-                target,
-                "rw" if writable else "ro",
-            )
-        )
+        # Mounts are writable by default, so 'rw' is optional and not
+        # supported (due to a bug) in some 3.6 series releases.
+        vol = f"{source}:{target}"
+        if not writable:
+            vol += ":ro"
+        runtime.append(vol)
 
     def add_file_or_directory_volume(
         self, runtime: List[str], volume: MapperEnt, host_outdir_tgt: Optional[str]
@@ -395,28 +392,28 @@ def create_runtime(
             runtime.append("--userns")
         else:
             runtime.append("--pid")
+
+        container_HOME: Optional[str] = None
         if is_version_3_1_or_newer():
+            # Remove HOME, as passed in a special way (restore it below)
+            container_HOME = self.environment.pop("HOME")
             runtime.append("--home")
             runtime.append(
                 "{}:{}".format(
                     os.path.realpath(self.outdir),
-                    self.builder.outdir,
+                    container_HOME,
                 )
             )
         else:
-            runtime.append("--bind")
-            runtime.append(
-                "{}:{}:rw".format(
-                    os.path.realpath(self.outdir),
-                    self.builder.outdir,
-                )
-            )
-        runtime.append("--bind")
-        runtime.append(
-            "{}:{}:rw".format(
-                os.path.realpath(self.tmpdir),
-                self.CONTAINER_TMPDIR,
+            self.append_volume(
+                runtime,
+                os.path.realpath(self.outdir),
+                self.environment["HOME"],
+                writable=True,
             )
+
+        self.append_volume(
+            runtime, os.path.realpath(self.tmpdir), self.CONTAINER_TMPDIR, writable=True
         )
 
         self.add_volumes(
@@ -447,4 +444,8 @@ def create_runtime(
 
         for name, value in self.environment.items():
             env[f"SINGULARITYENV_{name}"] = str(value)
+
+        if container_HOME:
+            # Restore HOME if we removed it above.
+            self.environment["HOME"] = container_HOME
         return (runtime, None)

cwltool/software_requirements.py

@@ -32,7 +32,12 @@
     """#!/bin/bash
 cat > modify_environment.bash <<'EOF'
 $handle_dependencies
-env > output_environment.bash
+# First try env -0
+if ! env -0 > "output_environment.dat" 2> /dev/null; then
+    # If that fails, use the python script.
+    # In some circumstances (see PEP 538) this will the add LC_CTYPE env var.
+    python3 "env_to_stdout.py" > "output_environment.dat"
+fi
 EOF
 python3 "run_job.py" "job.json" "modify_environment.bash"
 """

tests/env3.cwl

@@ -3,6 +3,7 @@ cwlVersion: v1.0
 class: CommandLineTool
 inputs: []
 baseCommand: env
+arguments: ["-0"]
 outputs:
   env:
     type: stdout

tests/env4.cwl

@@ -0,0 +1,19 @@
+#!/usr/bin/env cwl-runner
+cwlVersion: v1.0
+class: CommandLineTool
+
+requirements:
+  InitialWorkDirRequirement:
+    listing:
+      - entryname: env0.py
+        entry: |
+          import os
+          for k, v in os.environ.items():
+              print(f"{k}={v}", end="\0")
+
+inputs: []
+baseCommand: python3
+arguments: ["env0.py"]
+outputs:
+  env:
+    type: stdout

tests/test_dependencies.py

@@ -1,17 +1,21 @@
 """Tests of satisfying SoftwareRequirement via dependencies."""
-from io import StringIO
-import json
 import os
 from pathlib import Path
 from shutil import which
 from types import ModuleType
-from typing import Optional
+from typing import Dict, List, Optional
 
 import pytest
 
 from cwltool.main import main
 
-from .util import get_data, get_main_output, needs_docker, working_directory
+from .util import (
+    get_data,
+    get_main_output,
+    get_tool_env,
+    needs_docker,
+    working_directory,
+)
 
 deps = None  # type: Optional[ModuleType]
 try:
@@ -74,29 +78,15 @@ def test_modules_environment(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) ->
     monkeypatch.setenv(
         "MODULEPATH", os.path.join(os.getcwd(), "tests/test_deps_env/modulefiles")
     )
+    tool_env = get_tool_env(
+        tmp_path,
+        [
+            "--beta-dependency-resolvers-configuration",
+            get_data("tests/test_deps_env_modules_resolvers_conf.yml"),
+        ],
+        get_data("tests/env_with_software_req.yml"),
+    )
 
-    stdout = StringIO()
-    stderr = StringIO()
-    with working_directory(tmp_path):
-        rc = main(
-            argsl=[
-                "--beta-dependency-resolvers-configuration",
-                get_data("tests/test_deps_env_modules_resolvers_conf.yml"),
-                get_data("tests/env3.cwl"),
-                get_data("tests/env_with_software_req.yml"),
-            ],
-            stdout=stdout,
-            stderr=stderr,
-        )
-        assert rc == 0
-
-        output = json.loads(stdout.getvalue())
-        env_path = output["env"]["path"]
-        tool_env = {}
-        with open(env_path) as _:
-            for line in _:
-                key, val = line.split("=", 1)
-                tool_env[key] = val[:-1]
     assert tool_env["TEST_VAR_MODULE"] == "environment variable ends in space "
     tool_path = tool_env["PATH"].split(":")
     assert get_data("tests/test_deps_env/random-lines/1.0/scripts") in tool_path