Fixes Popen output to use universal newlines.

[FrankD412]

Addition of a start_process utility method to centralize how processes are opened. Fixes #142.

[tadesautels]

Something's wonky here.

When I take the dev branch and ONLY modify slurmscriptadapter.py such that the three Popen calls all have universal_newlines=True and NO OTHER CHANGES, everything runs fine for me. However, if I use this bugfix, my maestro crashes out about when it should be launching the conductor; sometimes it gets to the point where it's supposed to be calling the new start_process in maestro.py (after study setup, to start the conductor) and sometimes it doesn't.

Frank, can we take a look at this tomorrow?

Tom

[tadesautels]

In my usecase, I'm getting silent errors with this bugfix, but not if I only change the three Popen lines in slurmscriptadapter to use Popen(..., universal_newlines=True).

This needs to be debugged.

[jsemler]

@tadesautels, @FrankD412 - This can be reproduced with the LULESH example. The conductor call is logged (with the log level set to debug) and then the program exits.

[FrankD412]

I just tried to execute the command in ipython and weirdly I get a return code of 127 which indicates that the specified binary doesn't exist. However, running it on the command line manually yields execution. I'm not sure what the resulting difference is yet.

[FrankD412]

Ok, so weirdly -- joining the list items into a string causes this to work... the next weird thing is that doing that ubiquitously via the start_process method now causes the GitDependency class to error out with the following:

In [1]: from maestrowf.utils import start_process

In [2]: url = "https://github.com/LLNL/LULESH.git"

In [3]: path = "/Users/frank/Documents/Code/Python/maestrowf/sample_output/lulesh/lulesh_sample1_20180917-230533/LULESH"

In [4]: p = start_process(["git", "clone", url, path], shell=None)
---------------------------------------------------------------------------
OSError                                   Traceback (most recent call last)
<ipython-input-4-bbe85a54c182> in <module>()
----> 1 p = start_process(["git", "clone", url, path], shell=None)

/Users/frank/Documents/Code/Python/maestrowf/maestrowf/utils.pyc in start_process(cmd, cwd, env, shell)
    173     return Popen(c,
    174                  shell=shell, stdout=PIPE, stderr=PIPE,
--> 175                  universal_newlines=True)

/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.pyc in __init__(self, args, bufsize, executable, stdin, stdout, stderr, preexec_fn, close_fds, shell, cwd, env, universal_newlines, startupinfo, creationflags)
    708                                 p2cread, p2cwrite,
    709                                 c2pread, c2pwrite,
--> 710                                 errread, errwrite)
    711         except Exception:
    712             # Preserve original exception in case os.close raises.

/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.pyc in _execute_child(self, args, executable, preexec_fn, close_fds, cwd, env, universal_newlines, startupinfo, creationflags, shell, to_close, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite)
   1333                         raise
   1334                 child_exception = pickle.loads(data)
-> 1335                 raise child_exception
   1336
   1337

OSError: [Errno 2] No such file or directory

I've reproduced the exact error repeatedly in ipython, the only thing that seems to fix it is specifying shell=True. The interesting thing is that if I specify shell=False which is what its default value would be when unset, I get the error as well. The issue seems to come from joining the command, because when I pass the list like I had before it works just fine. If I join the arguments and then run it I get the above error -- it almost seems like the string version of the command is only useable when dealing with shell=True.

[FrankD412]

Ah, I'm willing to bet it's because I now pass in the conductor command as a list which falls through because shell=True.

[FrankD412]

Yep -- that seems to be the case... I've made a small tweak to fix the issue, but now I wonder what the appropriate convention on this issue is...

[FrankD412]

It also looks like we can't avoid shell=True for launching the conductor. If I make the command a list and force shell=False, the Popen call returns 127 (executable not found). This implies to me that the shell is required because it's the loading of the shell environment that tells the system where to point for Python packages. I'm trying to think of a good middle group here to make it clear instead of overloading the start_process to account for every single case.

[tadesautels]

There's a bug in this one too; start_process isn't passing down cwd or env correctly. One possible method:

` kwargs_to_Popen = {"shell": shell}
if cwd is not None:
kwargs_to_Popen["cwd"] = cwd
if env is not None:
kwargs_to_Popen["env"] = env

return Popen(cmd,
             stdout=PIPE, stderr=PIPE,
             universal_newlines=True,
             **kwargs_to_Popen)`

[FrankD412]

Oh, you're right. I don't know if you can do the mixed declaration for calling Popen like that. I'll give it a shot -- but if that works I think that's the best path forward.

[tadesautels]

The mixed call worked for me.

[FrankD412]

Cool -- I'll go ahead and implement it. Learned something today!

[FrankD412]

So one thought that came up -- I've already flagged that if the specified cmd to Popen is a list we automatically set shell = False (not sure on the justification for it, but Popen raises an error). Generally, it's a no-no to silently change things under the hood without a user's knowledge. It sounds reasonable to me to throw an exception if there's a mismatch. Just wanted to field that as a thought for feedback.

[FrankD412]

My take on this:

def start_process(cmd, cwd=None, env=None, shell=True):
    """
    Starts a new process using a specified command.

    :param cmd: A string or a list representing the command to be run.
    :param cwd: Current working path that the process will be started in.
    :param env: A dictionary containing the environment the process will use.
    :param shell: Boolean that determines if the process will run a shell.
    """
    if isinstance(cmd, list):
        shell = False

    # Define kwargs for the upcoming Popen call.
    kwargs = {
        "shell":                shell,
        "cwd":                  cwd,
        "env":                  env,
        "universal_newlines":   True,
        "stdout":               PIPE,
        "stderr":               PIPE,
    }

    return Popen(cmd, **kwargs)

Though your code has the benefit of not passing in extra parameters.

[kcathey]

Pulling start_process is a good move to consolidate and standardize issues along these lines.