bpo-35346, platform: replace os.popen() with subprocess by vstinner · Pull Request #10786 · python/cpython

vstinner · 2018-11-29T01:44:01Z

Replace os.popen() with subprocess.Popen in the platform module:

Pass the command as a list of strings, not as as a string;
Don't use a shell;
_syscmd_ver() now redirects errors to DEVNULL.

https://bugs.python.org/issue35346

serhiy-storchaka

Can subprocess.check_output() be used here?

vstinner · 2018-11-29T09:00:42Z

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().

serhiy-storchaka · 2018-11-29T09:24:26Z

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.

malemburg · 2018-11-29T09:40:07Z

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

vstinner · 2018-11-29T11:24:46Z

+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.

serhiy-storchaka · 2018-11-29T11:32:52Z

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

serhiy-storchaka

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

vstinner · 2018-11-29T11:49:29Z

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().

vstinner · 2018-11-29T12:00:39Z

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?

serhiy-storchaka · 2018-11-29T12:04:34Z

The code contradicts the docstring. This can lead to returning an incorrect result if the executable path contains some key strings like "'32-bit'" or "PE". This is a separate issue, but it could be better to fix it before merging this PR. Merging this PR will make backporting harder.

It has been changed by commit 685fffa, https://bugs.python.org/issue16112 The docstring seems to be outdated.

This can lead to returning an incorrect result if the executable path contains some key strings like "'32-bit'" or "PE".

I would prefer to keep this PR simple, and so don't try to fix this bug nor update the docstring.

@serhiy-storchaka created https://bugs.python.org/issue35348

vstinner · 2018-11-29T14:25:21Z

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

serhiy-storchaka · 2018-11-29T14:39:13Z

Why an AttributeError is caught here?

AttributeError is to catch... missing os.popen() function (I guess) :-) AttributeError is as old as the module itself: commit 246d847. Do you think that we should remove it?

I think it is better to remove it.

Guys, please remember that these modules are used by other Python implementations as well. The AttributeError most likely came from this: having e.g. IronPython or Jython not having the needed os API available.

Other Python implementations are supposed the implement the same stdlib. Why would IronPython or Jython not have subprocess.check_output()? For example, if IronPython implements "Python 3.5", they should copy platform.py from CPython 3.5.

If they decide to not implement subprocess.check_output(), well, we can have a talk :-) In the meanwhile, PyPy does patch the stdlib, but they don't propose these patches upstream :-(

This used to be different... could be that we didn't listen to them.

In any case, see here for when the AttributeErrors were added: https://github.com/python/cpython/blob/2b665dbb53d02be09947116c1fe2ac002375d348/Lib/platform.py#L64

In Python 2 the module was meant to work across many different Python versions and platforms. It would be good to keep this feature in place, since it needs to be updated more frequently than the whole Python version stack (i.e. applications should be able to copy a more recent from from 3.7 to 3.5 and still have it work).

Well, subprocess and and os.popen() are now available available on all platforms. The situation became better in Python 3 ;-)

Note: PyPy3 has os.popen() and subprocess, and Jython and IronPython are both stuck at Python 2.7 ;-)

os.popen() is optional in Python 2, but always available in Python 3. If other implementation have enough different stdlib without os.popen() and subprocess.check_output(), it can have different platform module as well.

I believe it is safe to remove AttributeError in all Python 3 versions.

Ok. I removed the AttributeError. Anyway, if someone comes with an usecase where catching AttributeError makes sense, I have no problem to reintroduce it ;-) We can even revert this whole commit later if it causes any issue ;-)

serhiy-storchaka · 2018-11-29T14:43:17Z

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

serhiy-storchaka · 2018-11-29T21:42:34Z

Why not use just ['ver'] with shell=True?

Why not use just ['ver'] with shell=True?

I wanted to avoid shell=True whenever possible, but ok, let's use shell=True here.

serhiy-storchaka

LGTM if it LGT @malemburg.

vstinner · 2018-11-30T11:26:14Z

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.

vstinner · 2018-12-03T13:01:02Z

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

Replace os.popen() with subprocess.check_output() in the platform module: * platform.uname() (_syscmd_ver() helper function) now redirects stderr to DEVNULL; * Remove platform.DEV_NULL; * _syscmd_uname() and _syscmd_file() no longer catch AttributeError. The "except AttributeError:" was only needed in Python 2, when os.popen() was not always available. In Python 3, subprocess.check_output() is always available.

vstinner · 2018-12-05T22:41:05Z

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.

…0786)" This reverts commit 3a521f0.

pablogsal · 2018-12-07T11:45:00Z

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.

the-knights-who-say-ni added the CLA signed label Nov 29, 2018

bedevere-bot added the awaiting merge label Nov 29, 2018

vstinner added the skip news label Nov 29, 2018

serhiy-storchaka reviewed Nov 29, 2018

View reviewed changes

serhiy-storchaka approved these changes Nov 30, 2018

View reviewed changes

vstinner merged commit 3a521f0 into python:master Dec 7, 2018

bedevere-bot removed the awaiting merge label Dec 7, 2018

vstinner deleted the platform_popen branch December 7, 2018 10:10

python deleted a comment from bedevere-bot Dec 7, 2018

vstinner added a commit that referenced this pull request Dec 7, 2018

Revert "bpo-35346, platform: replace os.popen() with subprocess (GH-1…

9cddd1c

…0786)" This reverts commit 3a521f0.

Uh oh!

Conversation

vstinner commented Nov 29, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 29, 2018

Uh oh!

serhiy-storchaka commented Nov 29, 2018

Uh oh!

malemburg commented Nov 29, 2018

Uh oh!

vstinner commented Nov 29, 2018

Uh oh!

serhiy-storchaka commented Nov 29, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 29, 2018

Uh oh!

vstinner commented Nov 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Nov 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 30, 2018

Uh oh!

vstinner commented Dec 3, 2018

Uh oh!

vstinner commented Dec 5, 2018

Uh oh!

pablogsal commented Dec 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vstinner commented Nov 29, 2018 •

edited by bedevere-bot

Loading