gh-87901: os.popen: Fix new encoding argument.

[methane]

• Make it keyword only.
• Fix it wasn't passed to subprocess.Popen

#87901

[hauntsaninja]

Looks good, thanks for fixing!

[JelleZijlstra]

You should also add a unit test that uses this new parameter.

[serhiy-storchaka]

Is there a What's New entry?

[serhiy-storchaka]

It is not a test for os.popen. It works without explicit encoding, so no need to change it.

Instead look at test_popen. Needed tests for encoding and errors, and they should fail if revert changes in os.popen, so use non-ASCII data and encodings which give different result in comparison with common locale encodings: utf-8, latin1 or cp1252.

[vstinner]

Would it make sense to add a new test for the private attribute to check for the encoding? To check encoding="utf-8".

[serhiy-storchaka]

I would prefer not to rely on private attributes if possible. Checking the result of the decoding is enough.

[vstinner]

Would it make sense to add a new test for the private attribute to check for the encoding? To check encoding="utf-8".

[serhiy-storchaka]

There is a dedicated test Lib/test/test_popen.py for this.

Test:

• Non-ASCII output with default encoding and errors.
• Non-ASCII output with different values of encoding.
• Non-ASCII output with a specified encoding and different values of errors.
• Non-ASCII command arguments and how they interact with the specified encoding.

Include tests for the locale encoding and the filesystem encoding if they are relevant.

[methane]

os.popen() is just a simple wrapper of subprocess.Popen()
And since os.popen() uses shell=True with string cmd, writing/maintain such a test in cross platform is 100x difficult than its implementation.

[methane]

Or should we revert the encoding argument and recommend using subprocess instead?

[vstinner]

os.popen() is just a simple wrapper of subprocess.Popen()

I dislike this API since it always use a shell and so is more likely to be vulnerable to shell code injection. It may also be less efficient than avoiding a shell. I would prefer to deprecate this function, but I was never brave enough to propose deprecating it. At least, I removed platform.popen() ;-)

[methane]

I created #92836 that remove encoding argument and add document instead.