Merge pull request #1326 from ESMCI/jgfouca/refactor_run_cmd

Refactor run_cmd to handle file output streams more easily
This will allow users to redirect output to files without
having to rely on shell tricks.

Test suite: scripts_regression_tests
Test baseline:
Test namelist changes:
Test status: bit for bit

User interface changes?: None

Code review: @jedwards4b

scripts/lib/CIME/SystemTests/homme.py

@@ -33,10 +33,10 @@ def build_phase(self, sharedlib_only=False, model_only=False):
             gmake    = self._case.get_value("GMAKE")
 
             basename = basegen if generate else basecmp
-            cmake_cmd = "cmake -C %s/components/homme/cmake/machineFiles/%s.cmake -DUSE_NUM_PROCS=%s %s/components/homme -DHOMME_BASELINE_DIR=%s/%s >& homme.bldlog" % (srcroot, mach, procs, srcroot, baseline, basename)
+            cmake_cmd = "cmake -C %s/components/homme/cmake/machineFiles/%s.cmake -DUSE_NUM_PROCS=%s %s/components/homme -DHOMME_BASELINE_DIR=%s/%s" % (srcroot, mach, procs, srcroot, baseline, basename)
 
-            run_cmd_no_fail(cmake_cmd, from_dir=exeroot)
-            run_cmd_no_fail("%s -j8 >> homme.bldlog 2>&1" % gmake, from_dir=exeroot)
+            run_cmd_no_fail(cmake_cmd, arg_stdout="homme.bldlog", combine_output=True, from_dir=exeroot)
+            run_cmd_no_fail("%s -j8" % gmake, arg_stdout="homme.bldlog", combine_output=True, from_dir=exeroot)
 
             post_build(self._case, [os.path.join(exeroot, "homme.bldlog")])
 
@@ -56,14 +56,14 @@ def run_phase(self):
 
         if generate:
             full_baseline_dir = os.path.join(baseline, basegen, "tests", "baseline")
-            run_cmd_no_fail("%s -j 4 baseline >& %s" % (gmake, log), from_dir=exeroot)
+            run_cmd_no_fail("%s -j 4 baseline" % gmake, arg_stdout=log, combine_output=True, from_dir=exeroot)
             if os.path.isdir(full_baseline_dir):
                 shutil.rmtree(full_baseline_dir)
             shutil.copytree(os.path.join(exeroot, "tests", "baseline"), full_baseline_dir)
         elif compare:
-            run_cmd_no_fail("%s -j 4 check >& %s" % (gmake, log), from_dir=exeroot)
+            run_cmd_no_fail("%s -j 4 check" % gmake, arg_stdout=log, combine_output=True, from_dir=exeroot)
         else:
-            run_cmd_no_fail("%s -j 4 baseline >& %s" % (gmake, log), from_dir=exeroot)
+            run_cmd_no_fail("%s -j 4 baseline" % gmake, arg_stdout=log, combine_output=True, from_dir=exeroot)
 
         # Add homme.log output to TestStatus.log so that it can
         # appear on the dashboard. Otherwise, the TestStatus.log

scripts/lib/CIME/XML/env_batch.py

@@ -384,7 +384,7 @@ def submit_single_job(self, case, job, depid=None, no_batch=False, batch_args=No
                 submitcmd += string + " "
 
         logger.info("Submitting job script %s"%submitcmd)
-        output = run_cmd_no_fail(submitcmd + " 2>&1")
+        output = run_cmd_no_fail(submitcmd, combine_output=True)
         jobid = self.get_job_id(output)
         logger.info("Submitted job id is %s"%jobid)
         return jobid

scripts/lib/CIME/XML/env_mach_specific.py

@@ -108,7 +108,7 @@ def list_modules(self):
             source_cmd = ""
 
         if (module_system == "module"):
-            return run_cmd_no_fail("%smodule list 2>&1" % source_cmd)
+            return run_cmd_no_fail("%smodule list" % source_cmd, combine_output=True)
         elif (module_system == "soft"):
             # Does soft really not provide this capability?
             return ""
@@ -126,7 +126,7 @@ def save_all_env_info(self, filename):
         """
         with open(filename, "w") as f:
             f.write(self.list_modules())
-        run_cmd_no_fail("echo -e '\n' >> %s && env >> %s" % (filename, filename))
+        run_cmd_no_fail("echo -e '\n' && env", arg_stdout=filename)
 
     def make_env_mach_specific_file(self, compiler, debug, mpilib, shell):
         modules_to_load = self._get_modules_for_case(compiler, debug, mpilib)

scripts/lib/CIME/build.py

@@ -254,10 +254,9 @@ def _build_libraries(case, exeroot, sharedpath, caseroot, cimeroot, libroot, lid
             my_file = "PYTHONPATH=%s:%s:$PYTHONPATH %s"%(os.path.join(cimeroot,"scripts","Tools"),
                                                           os.path.join(cimeroot,"scripts","lib"), my_file)
         logger.info("Building %s with output to file %s"%(lib,file_build))
-        with open(file_build, "w") as fd:
-            stat = run_cmd("%s %s %s %s 2>&1" %
-                           (my_file, full_lib_path, os.path.join(exeroot,sharedpath), caseroot),
-                           from_dir=exeroot,arg_stdout=fd)[0]
+        stat = run_cmd("%s %s %s %s" %
+                       (my_file, full_lib_path, os.path.join(exeroot,sharedpath), caseroot),
+                       from_dir=exeroot, combine_output=True, arg_stdout=file_build)[0]
 
         analyze_build_log(lib, file_build, compiler)
         expect(stat == 0, "ERROR: buildlib.%s failed, cat %s" % (lib, file_build))

scripts/lib/CIME/hist_utils.py

@@ -263,10 +263,14 @@ def cprnc(model, file1, file2, case, rundir, multiinst_cpl_compare=False, outfil
     if mstr1 != mstr2:
         mstr = mstr1+mstr2
 
-    output_filename = "%s%s.cprnc.out"%(basename, mstr)
+    output_filename = os.path.join(rundir, "%s%s.cprnc.out" % (basename, mstr))
     if outfile_suffix:
-        output_filename += ".%s"%(outfile_suffix)
-    cpr_stat, out, _ = run_cmd("%s -m %s %s 2>&1 | tee %s/%s" % (cprnc_exe, file1, file2, rundir, output_filename))
+        output_filename += ".%s" % outfile_suffix
+
+    cpr_stat = run_cmd("%s -m %s %s" % (cprnc_exe, file1, file2), combine_output=True, arg_stdout=output_filename)[0]
+    with open(output_filename, "r") as fd:
+        out = fd.read()
+
     if multiinst_cpl_compare:
         #  In a multiinstance test the cpl hist file will have a different number of
         # dimensions and so cprnc will indicate that the files seem to be DIFFERENT

scripts/lib/CIME/provenance.py

@@ -35,7 +35,7 @@ def save_build_provenance_acme(case, lid=None):
     describe_prov = os.path.join(exeroot, "GIT_DESCRIBE.%s" % lid)
     if os.path.exists(describe_prov):
         os.remove(describe_prov)
-    run_cmd_no_fail("git describe > %s" % describe_prov, from_dir=cimeroot)
+    run_cmd_no_fail("git describe", arg_stdout=describe_prov, from_dir=cimeroot)
 
     # Save HEAD
     headfile = os.path.join(cimeroot, ".git", "logs", "HEAD")
@@ -119,12 +119,12 @@ def save_prerun_provenance_acme(case, lid=None):
     if mach == "mira":
         for cmd, filename in [("qstat -lf", "qstatf"), ("qstat -lf %s" % job_id, "qstatf_jobid")]:
             filename = "%s.%s" % (filename, lid)
-            run_cmd_no_fail("%s > %s" % (cmd, filename), from_dir=full_timing_dir)
+            run_cmd_no_fail(cmd, arg_stdout=filename, from_dir=full_timing_dir)
             gzip_existing_file(os.path.join(full_timing_dir, filename))
     elif mach in ["edison", "cori-haswell", "cori-knl"]:
         for cmd, filename in [("sqs -f", "sqsf"), ("sqs -w -a", "sqsw"), ("sqs -f %s" % job_id, "sqsf_jobid"), ("squeue", "squeuef")]:
             filename = "%s.%s" % (filename, lid)
-            run_cmd_no_fail("%s > %s" % (cmd, filename), from_dir=full_timing_dir)
+            run_cmd_no_fail(cmd, arg_stdout=filename, from_dir=full_timing_dir)
             gzip_existing_file(os.path.join(full_timing_dir, filename))
     elif mach == "titan":
         for cmd, filename in [("xtdb2proc -f", "xtdb2proc"),
@@ -137,7 +137,7 @@ def save_prerun_provenance_acme(case, lid=None):
             gzip_existing_file(os.path.join(full_timing_dir, filename + "." + lid))
 
         mdiag_reduce = os.path.join(full_timing_dir, "mdiag_reduce." + lid)
-        run_cmd_no_fail("./mdiag_reduce.csh > %s" % mdiag_reduce, from_dir=os.path.join(caseroot, "Tools"))
+        run_cmd_no_fail("./mdiag_reduce.csh", arg_stdout=mdiag_reduce, from_dir=os.path.join(caseroot, "Tools"))
         gzip_existing_file(mdiag_reduce)
 
     # copy/tar SourceModes
@@ -190,9 +190,9 @@ def save_prerun_provenance_acme(case, lid=None):
 
     # Save state of repo
     if os.path.exists(os.path.join(cimeroot, ".git")):
-        run_cmd_no_fail("git describe > %s" % os.path.join(full_timing_dir, "GIT_DESCRIBE.%s" % lid), from_dir=cimeroot)
+        run_cmd_no_fail("git describe", arg_stdout=os.path.join(full_timing_dir, "GIT_DESCRIBE.%s" % lid), from_dir=cimeroot)
     else:
-        run_cmd_no_fail("git describe > %s" % os.path.join(full_timing_dir, "GIT_DESCRIBE.%s" % lid), from_dir=os.path.dirname(cimeroot))
+        run_cmd_no_fail("git describe", arg_stdout=os.path.join(full_timing_dir, "GIT_DESCRIBE.%s" % lid), from_dir=os.path.dirname(cimeroot))
 
 def save_prerun_provenance_cesm(case, lid=None): # pylint: disable=unused-argument
     pass

scripts/lib/CIME/test_scheduler.py

@@ -325,7 +325,7 @@ def _update_test_status(self, test, phase, status):
     def _shell_cmd_for_phase(self, test, cmd, phase, from_dir=None):
     ###########################################################################
         while True:
-            rc, output, _ = run_cmd(cmd + " 2>&1", from_dir=from_dir)
+            rc, output, _ = run_cmd(cmd, combine_output=True, from_dir=from_dir)
             if rc != 0:
                 self._log_output(test,
                                  "%s FAILED for test '%s'.\nCommand: %s\nOutput: %s\n" %
@@ -545,7 +545,7 @@ def _setup_phase(self, test):
         rv = self._shell_cmd_for_phase(test, "./case.setup", SETUP_PHASE, from_dir=test_dir)
 
         # It's OK for this command to fail with baseline diffs but not catastrophically
-        cmdstat, output, _ = run_cmd("./case.cmpgen_namelists 2>&1", from_dir=test_dir)
+        cmdstat, output, _ = run_cmd("./case.cmpgen_namelists", combine_output=True, from_dir=test_dir)
         expect(cmdstat in [0, TESTS_FAILED_ERR_CODE], "Fatal error in case.cmpgen_namelists: %s" % output)
 
         return rv

scripts/lib/CIME/utils.py

@@ -143,9 +143,15 @@ def get_model():
                       and model != "xml_schemas"])
     expect(False, msg)
 
+def _convert_to_fd(filearg, from_dir):
+    if not filearg.startswith("/") and from_dir is not None:
+        filearg = os.path.join(from_dir, filearg)
+
+    return open(filearg, "a")
+
 _hack=object()
 def run_cmd(cmd, input_str=None, from_dir=None, verbose=None,
-            arg_stdout=_hack, arg_stderr=_hack, env=None):
+            arg_stdout=_hack, arg_stderr=_hack, env=None, combine_output=False):
     """
     Wrapper around subprocess to make it much more convenient to run shell commands
 
@@ -155,10 +161,15 @@ def run_cmd(cmd, input_str=None, from_dir=None, verbose=None,
     import subprocess # Not safe to do globally, module not available in older pythons
 
     # Real defaults for these value should be subprocess.PIPE
-    if (arg_stdout is _hack):
+    if arg_stdout is _hack:
         arg_stdout = subprocess.PIPE
-    if (arg_stderr is _hack):
-        arg_stderr = subprocess.PIPE
+    elif isinstance(arg_stdout, str):
+        arg_stdout = _convert_to_fd(arg_stdout, from_dir)
+
+    if arg_stderr is _hack:
+        arg_stderr = subprocess.STDOUT if combine_output else subprocess.PIPE
+    elif isinstance(arg_stderr, str):
+        arg_stderr = _convert_to_fd(arg_stdout, from_dir)
 
     if (verbose != False and (verbose or logger.isEnabledFor(logging.DEBUG))):
         logger.info("RUN: %s" % cmd)
@@ -181,6 +192,12 @@ def run_cmd(cmd, input_str=None, from_dir=None, verbose=None,
     errput = errput.strip() if errput is not None else errput
     stat = proc.wait()
 
+    if isinstance(arg_stdout, file):
+        arg_stdout.close() # pylint: disable=no-member
+
+    if isinstance(arg_stderr, file) and arg_stderr is not arg_stdout:
+        arg_stderr.close() # pylint: disable=no-member
+
     if (verbose != False and (verbose or logger.isEnabledFor(logging.DEBUG))):
         if stat != 0:
             logger.info("  stat: %d\n" % stat)
@@ -192,7 +209,7 @@ def run_cmd(cmd, input_str=None, from_dir=None, verbose=None,
     return stat, output, errput
 
 def run_cmd_no_fail(cmd, input_str=None, from_dir=None, verbose=None,
-                    arg_stdout=_hack, arg_stderr=_hack):
+                    arg_stdout=_hack, arg_stderr=_hack, env=None, combine_output=False):
     """
     Wrapper around subprocess to make it much more convenient to run shell commands.
     Expects command to work. Just returns output string.
@@ -207,14 +224,18 @@ def run_cmd_no_fail(cmd, input_str=None, from_dir=None, verbose=None,
 
     >>> run_cmd_no_fail('grep foo', input_str='foo')
     'foo'
+
+    >>> run_cmd_no_fail('echo THE ERROR >&2', combine_output=True)
+    'THE ERROR'
     """
-    stat, output, errput = run_cmd(cmd, input_str, from_dir, verbose, arg_stdout, arg_stderr)
+    stat, output, errput = run_cmd(cmd, input_str, from_dir, verbose, arg_stdout, arg_stderr, env, combine_output)
     if stat != 0:
         # If command produced no errput, put output in the exception since we
         # have nothing else to go on.
         errput = output if errput == "" else errput
         expect(False, "Command: '%s' failed with error '%s'%s" %
                (cmd, errput, "" if from_dir is None else " from dir '%s'" % from_dir))
+
     return output
 
 def check_minimum_python_version(major, minor):

scripts/lib/CIME/wait_for_tests.py

@@ -170,7 +170,7 @@ def create_cdash_upload_xml(results, cdash_build_name, cdash_build_group, utc_ti
             if (os.path.exists(tarball)):
                 os.remove(tarball)
 
-            CIME.utils.run_cmd_no_fail("tar -cf - %s | gzip -c > %s" % (log_dir, tarball))
+            CIME.utils.run_cmd_no_fail("tar -cf - %s | gzip -c" % log_dir, arg_stdout=tarball)
             base64 = CIME.utils.run_cmd_no_fail("base64 %s" % tarball)
 
             xml_text = \