Fix run on non-Windows environments

[kalkie]

• Simplifies subprocess spawning, getting rid of the WindowsError and still supporting builtins and Window's file associations for file launch.
• Use shutil.which for more accurate executable finding.

Fixes #18.

[techalchemy]

Thanks for the pr!

The fallbacks and approaches we are using are there for a reason. That is to say, we have them in place because we have implemented and deployed the alternative approach and run into edge cases

So, while I appreciate that this results in less code, the existing approach (which explains why it is in place) is quite necessary.

Also, shutil.which has a number of issues which prevent it from being cross platform. I actually have a fix in place for this, I have simply avoided merging my PR until I am sure it doesn't cause problems in pipenv (I am not sure of that yet)

[kalkie]

@techalchemy Can you elaborate what these edge cases are, so I can verify it behaves correctly after my changes. I tested this quite extensively with Python 2.7/3.7 and Mac OS/Windows. Running an executable, shell builtin or Windows app based on file associations works like before. Also, the behavior if the command fails to execute is now consistent between Windows and others, i.e. no exception is thrown but run returns the error message.

[techalchemy]

I tested this quite extensively with Python 2.7/3.7 and Mac OS/Windows

We test against 2.7 through 3.7 including 3.4, 3.5 and 3.6, on windows, osx and linux, and there is a discussion about this here: pypa/pipenv#2882

We also test this in production systems across every user of pipenv, and run into those edge cases, which is how we know about them, and is also why we aren't likely to simply make a change because you did some testing locally :|

The scale of the usage of these libraries is enormous, so just because it works for you doesn't mean it covers all of the edge cases. I appreciate your contribution, but you should start by assuming the approach we took has a reason behind it, and ask yourself what that reason is, or if you're not sure, ask us. It is a lot more productive than assuming our code is wrong and we had no reason. "It works on my machine" is unfortunately not usually good enough in cases like this one.

[kalkie]

@techalchemy Thanks for your responses! I understand your concern, but please let me point a few things before you make the decision.

If I've understood correctly, the changes I've simplified here, which came mostly in commit 1232e7b, were motivated by an improvement done originally for pipenv (pypa/pipenv/pull/2727). The intention was to support Windows' file launch and not to raise OSError if execution failed.

However, the original change in pipenv didn't use distutils.spawn.find_executable, but it used which/where system utility. On Windows, where returns all kinds of things, not just executables, as was noted in the pipenv PR comment. This was the reason why the original author had to execute the command first and catch WindowsError if it wasn't an executable after all. Also, the original change was in a code path specific to Windows.

Now, when these changes were reproduced in vistir, the system_which was changed to distutils' find_executable, which only returns paths ending with .exe on Windows. Please see the relevant part in distutils' source. In other words, if you feed script.py to find_executable, it will return script.py.exe if found from PATH, or None. Because of this, the vistir version will use the shell mode for .py files (or anything not ending with .exe) already on the first execution because of this check. This renders the WindowsError catching almost useless.

The one place where the current dual execution would make a difference is if the command actually ends with .exe, or looks like an executable in others systems, but would still not be a valid application. In this case the first execution would result in WindowsError, or more generally OSError. Then the next execution would be done in shell mode, producing a popup on Windows that the user would have to manually click and then returns an error string Access is denied. However, with the simplified logic in this PR, _spawn_subprocess would raise an OSError. If you want to return an error string in this case as well, that could be handled in _create_subprocess with something like this:

  try:
      c = _spawn_subprocess(cmd, env=env, block=block, cwd=cwd,
                                                  combine_stderr=combine_stderr)
+ except OSError as exc:
+     if return_object:
+         raise  # there is no object to return
+     return '', str(exc)
  except Exception as exc:
      print("Error %s while executing command %s", exc, " ".join(cmd._parts))
      raise

That would also prevent the popup and result in a more descriptive error message.

I changed to shutil.which, because it also checks if the file is executable on systems where it's possible. This works with distutils' find_executable as well, it'll just return more false positives.

I hope you had the time to read this far. If you still think this doesn't make any sense, then I'll just go away.. :-)

[techalchemy]

It does make sense to me as described, that is nice digging! Thanks for persisting. I’d accept a PR for that for sure