Can subprocess.check_output() be used here?

This function is documented in the "Older high-level API" section of the subprocess documentation. It seems like subprocess.run() is now preferred. subprocess.run() kills the process on exception, IMHO it's a nice enhancement (to not leak a process on error). I modified my PR to use subprocess.run().

Should not check=True be passed? And check_output() looks as a more convenient specialized function for this case. You can omit stdout=PIPE and check=True.

+1 on using .check_output() instead. It's a special purpose API for exactly this use case and also available in Python 2.7.

+1 on using .check_output() instead. It's a special purpose API for exactly this use case and also available in Python 2.7.

I don't plan to backport this change to other branches.

check_output() / check=True.

I chose to keep the code checking manually the returncode. I don't see the point of a raising an exception, since the exception is ignored anyway.

I think the code would be simpler, and therefore more readable, if use check_output().

I think the code would be simpler, and therefore more readable, if use check_output().

done

Visible effect: suppressed the output to stderr. I think it is worth a news entry.

done

And it can be worth backporting.

My PR only changes _syscmd_ver() when os.uname() doesn't exist, so only impacts Windows. I don't recall that I ever saw any spurious error when using platform on Windows.

I would prefer to minimize changes in stable branches. So if someone considers that it's worth it (I don't think so), I suggest to add " 2> %s" % DEV_NULL to _syscmd_ver() command, rather than using check_output().

Hum, my PR didn't work as expected on Windows. Currently, _syscmd_ver() tries to run "ver" (as a string) with os.open() which uses a shell. My PR avoids shell=True. _syscmd_ver() still worked thanks to "cmd /c ver"... but that's basically like calling "ver" with shell=True, since subprocess uses cmd.exe as the shell:

            if shell:
                startupinfo.dwFlags |= _winapi.STARTF_USESHOWWINDOW
                startupinfo.wShowWindow = _winapi.SW_HIDE
                comspec = os.environ.get("COMSPEC", "cmd.exe")
                args = '{} /c "{}"'.format (comspec, args)

subprocess may not use cmd.exe if COMSPEC is set, but cmd.exe should always be available on Windows, no?

@malemburg, @serhiy-storchaka: Does my PR look better with check_output()?

Thank you, it looks better. But please defer merging it until issues with _syscmd_file() be resolved.

Marc Andre Lemburg:

+1 on using .check_output() instead. It's a special purpose API for exactly this use case and also available in Python 2.7.

@malemburg: My PR now uses check_output() as you requested. Would you mind to review it and make sure that you agree with the change?

Note: It hasn't been mentioned explicitely previously, but my change removes platform.DEV_NULL. This constant is not documented. Since Python 3 has os.devnull and subprocess.DEVNULL, I don't think that it's worth it to keep platform.DEV_NULL for backward compatibility.

@malemburg: I plan to merge this PR at the end of the week.

I squashed the 7 commits into a single commit to update the commit message, I rebased my PR and I also replaced os.open() with subprocess.check_output() in test_platform.

I removed the comments of the buildbot failures because they were false positives due to bad git checkouts. I am investigating why this PR that we made to fix that problem did not work.