From 8aaea714b9ff286aa0eec7df71d80e2293b5aafd Mon Sep 17 00:00:00 2001 From: James Foucar Date: Mon, 10 Apr 2017 09:48:10 -0600 Subject: [PATCH] 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. --- scripts/lib/CIME/SystemTests/homme.py | 12 ++++----- scripts/lib/CIME/XML/env_batch.py | 2 +- scripts/lib/CIME/XML/env_mach_specific.py | 4 +-- scripts/lib/CIME/build.py | 7 +++-- scripts/lib/CIME/hist_utils.py | 10 ++++--- scripts/lib/CIME/provenance.py | 12 ++++----- scripts/lib/CIME/test_scheduler.py | 4 +-- scripts/lib/CIME/utils.py | 33 ++++++++++++++++++----- scripts/lib/CIME/wait_for_tests.py | 2 +- 9 files changed, 55 insertions(+), 31 deletions(-) diff --git a/scripts/lib/CIME/SystemTests/homme.py b/scripts/lib/CIME/SystemTests/homme.py index 23661bfe377..e32c01e7599 100644 --- a/scripts/lib/CIME/SystemTests/homme.py +++ b/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 diff --git a/scripts/lib/CIME/XML/env_batch.py b/scripts/lib/CIME/XML/env_batch.py index 4f92b396e41..88fcdfc8f21 100644 --- a/scripts/lib/CIME/XML/env_batch.py +++ b/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 diff --git a/scripts/lib/CIME/XML/env_mach_specific.py b/scripts/lib/CIME/XML/env_mach_specific.py index 01a83c31a0c..b3efbb54bba 100644 --- a/scripts/lib/CIME/XML/env_mach_specific.py +++ b/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) diff --git a/scripts/lib/CIME/build.py b/scripts/lib/CIME/build.py index e7ac3758a6d..3c5a93ccf0e 100644 --- a/scripts/lib/CIME/build.py +++ b/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)) diff --git a/scripts/lib/CIME/hist_utils.py b/scripts/lib/CIME/hist_utils.py index 3d43ff4118d..5ae1217f57c 100644 --- a/scripts/lib/CIME/hist_utils.py +++ b/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 diff --git a/scripts/lib/CIME/provenance.py b/scripts/lib/CIME/provenance.py index 4da851dc4f5..26767495205 100644 --- a/scripts/lib/CIME/provenance.py +++ b/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 diff --git a/scripts/lib/CIME/test_scheduler.py b/scripts/lib/CIME/test_scheduler.py index b0b4c231626..b3f977fc988 100644 --- a/scripts/lib/CIME/test_scheduler.py +++ b/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 diff --git a/scripts/lib/CIME/utils.py b/scripts/lib/CIME/utils.py index 11d22984c54..d0e362cbd28 100644 --- a/scripts/lib/CIME/utils.py +++ b/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): diff --git a/scripts/lib/CIME/wait_for_tests.py b/scripts/lib/CIME/wait_for_tests.py index 1bbf6292994..efc850748d1 100644 --- a/scripts/lib/CIME/wait_for_tests.py +++ b/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 = \