On 4 December 2010 18:14, Gregory P. Smith wrote:

On Sat, Dec 4, 2010 at 3:45 AM, Antoine Pitrou wrote:

...

On Sat,  4 Dec 2010 10:10:44 +0100 (CET) gregory.p.smith wrote:

...

Author: gregory.p.smith Date: Sat Dec  4 10:10:44 2010 New Revision: 87010

Log: issue7213 + issue2320: Cause a DeprecationWarning if the close_fds argument is not passed to subprocess.Popen as the default value will be changing in a future Python to the safer and more often desired value of True.

That doesn't seem to be a good idea under Windows, is it?

(“Note that on Windows, you cannot set *close_fds* to true and also redirect the standard handles by setting *stdin*, *stdout* or *stderr*.”)

Regardless of what platform you are on I think the warning makes sense, it was just worded too strongly.  It is asking people to be explicit with close_fds.  Explicitly setting close_fds=False when that is desired is good. I modified the docs and warning message to just say that the default close_fds behavior will change in the future without specifying exactly what it will be.  It could make sense for the default to be a soft-True on windows that changes behavior if any of the std handles are set if that matches what users expect and find easiest.  We may want to reconsider its entire future in the face of the new pass_fds parameter, etc.  Those are 3.3 decisions.

This sounds like omitting the close_fds parameter is now considered ill-advised, if not outright wrong. That's silly - I certainly never specify close_fds, expecting the module to do the correct thing if I don't know/care enough to say. I use Popen as a convenience function, so ignoring low-level details is the whole point in my opinion (and close_fds *is* a low-level detail for what I do, on Windows). At the very least, the whole of the section "Replacing Older Functions with the subprocess Module" (with a couple of small exceptions) is in need of updating, as it is recommending bad practice (not specifying close_fds) at the moment... OK, I'm exaggerating a touch here. But can someone point me at the discussion of this change? Or if there hasn't been one, can we have one (i.e., to start the ball rolling, can someone justify the change, please). Paul.

On 4 December 2010 20:13, Gregory P. Smith wrote:

...

This sounds like omitting the close_fds parameter is now considered ill-advised, if not outright wrong. [...] Making the change was intended to force the discussion.  I'm glad that worked. :)

:-)

I don't like the thought of requiring people to specify close_fds either but the default is a bad one (see http://bugs.python.org/issue7213 and http://bugs.python.org/issue2320) so we should change it.

Both of these issues seem to be for Unix (based on the fact that they use grep and cat as executables - I haven't dug very deeply into this). I work on Windows only, so I'm happy to take the experts' opinions on that OS. Is there an issue on Windows? If not, and given how different FD inheritance is on Windows, I'd argue that in the absence of bug reports, there's no need to change behaviour on Windows.

The real question is how should we go about doing the change?  This warning asking people to always specify close_fds=True is heavy handed.  Other places in the stdlib and docs no doubt still need to be updated to reflect it, etc.

Options that seem worthy of debate: A) The DeprecationWarning that exists in py3k as of today.

Given that the option appears not to be getting deprecated (just the default changed), this seems wrong. I know DeprecationWarnings are off by default. If someone switches them on, they'd expect to have to address them. But what should they do in this case? Unless they have thought hard about FD inheritance, and then explicitly decided to use the default because it matches what they want (as opposed to specifying it explicitly, and documenting their intent clearly), nothing. Or they use the default expecting it to be good enough, and suddenly have to worry if they are relying on something that's changing (I have no feel for how likely it is that they'll be affected by the change, other than to say that personally, none of my code would be). That seems to me like a bad use of a DeprecationWarning, as a non-trivial amount of the time there's nothing to do.

B) Remove the DeprecationWarning, simply document that people should be explicit about it if they care at all and that the default will change in 3.3.

That seems reasonable to me. But I can see that the backward compatibility rules might apply here. On the other hand, given the bug reports, surely this is a bug fix?

C) Never change close_fds behavior.  we're stuck with it for life.

Sounds harsh. But look at it another way - if compatibility rules mean that we can't change the default, the reported bugs need to be fixed another way. Or behaviour needs to be documented more explicitly so that the bugs can be downgraded to "user error" caused by not reading the (new, improved) documentation. Compatibility rules are to protect working programs. What proportion of programs using the default are "working" and what proportion are "wrong"? It seems to me that most are "working", so compatibility applies.

D) Deprecate close_fds so that it becomes a legacy no-op.  the new pass_fds flag could evolve into this.  this will break existing code that depends on close_fds one way or another.

This works for me, as I never specify close_fds.

I'm in 100% agreement that forcing people to pass close_fds in makes the API less friendly (granted, people already find it unfriendly but why make it worse?).

I'd say making the parameter mandatory is highly unfriendly - to the point of making the API feel like an "advanced" one, not suitable for simple cases like replacing os.system.

Option B seems the most friendly to me. Option D is always on the table but I haven't planned out what a future without it should look like.  I prefer requiring people who need open file descriptors to pass them in; a semaphore for "all fds" could be created and pass_fds=ALL becomes the new "close_fds=False" (I think one of the bugs suggests this).

In my view: 1. Don't change Windows behaviour unless there's a demonstrable issue. The Popen code is split into 2 distinct paths internally anyway, so that shouldn't be a problem. If documenting the behaviour becomes awkward should Unix behaviour change and Windows doesn't, that indicates to me that there's a problem with the behaviour ("if it's hard to explain..."). 2. Users who don't specify close_fds because they don't care (as opposed to because they explicitly want the current default, but choose not to say so - I appreciate that this is a fine distinction, difficult to police in practice) should see no change in behaviour, apart from identified bugs being fixed. If you're not hitting a bug, you should see no change at all. Keep the interface straightforward for people who don't know (or don't want to worry about) the subtleties of FD inheritance. The original goal was for subprocess to replace os.system, os.popen, os.spawn, etc. That's never quite happened because subprocess is just a little bit too conceptually complex for those basic tasks. Let's not make it worse! Paul.

On Sat, 4 Dec 2010 23:17:49 +0000 Paul Moore wrote:

On 4 December 2010 23:07, Paul Moore wrote:

...

Is there an issue on Windows? If not, and given how different FD inheritance is on Windows, I'd argue that in the absence of bug reports, there's no need to change behaviour on Windows.

Actually, from the error message I just got:

ValueError: close_fds is not supported on Windows platforms if you redirect stdin/stdout/stderr

So on Windows, for the issues mentioned (both of which involve redirected stdin/out/err), leaving the current default appears to be not only advisable, but required.

I suspect an os-dependent default would be ugly to document...

How about a best-effort behaviour? Setting close_fds to True would only close fds where possible (i.e., not under Windows when piping either of stdin, stdout, stderr). Regards Antoine.

On 5 December 2010 14:20, Antoine Pitrou wrote:

How about a best-effort behaviour? Setting close_fds to True would only close fds where possible (i.e., not under Windows when piping either of stdin, stdout, stderr).

Is that plausible? I thought that it's possible to close fds, but doesn't necessarily do the right thing. If it's possible to do this, I'd be OK with it, but if it could cause problems just as subtle as the ones we already have, I don't see the benefit. Paul.

Glenn> On 12/4/2010 3:07 PM, Paul Moore wrote: >> The original goal was for subprocess to replace os.system, os.popen, >> os.spawn, etc. That's never quite happened because subprocess is just >> a little bit too conceptually complex for those basic tasks. Glenn> Is that way? I didn't find it particularly hard to learn, given Glenn> the "cheat sheet" of techniques for doing the replacements. For 99% of my usage (I suspect for most other peoples' as well, at least on Unix-y systems), this is all I need: for line in os.popen("some pipeline"): do_stuff(line) No cheat sheet necessary. I don't see how subprocess could have made that common idiom any simpler. Maybe it's better at doing esoteric stuff, however that falls into the 1% where a simple os.popen isn't adequate. Skip

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 12/04/2010 03:13 PM, Gregory P. Smith wrote:

On Sat, Dec 4, 2010 at 11:28 AM, Paul Moore wrote:

...

On 4 December 2010 18:14, Gregory P. Smith wrote:

...

On Sat, Dec 4, 2010 at 3:45 AM, Antoine Pitrou

wrote:

...

...

On Sat, 4 Dec 2010 10:10:44 +0100 (CET) gregory.p.smith wrote:

...

Author: gregory.p.smith Date: Sat Dec 4 10:10:44 2010 New Revision: 87010

Log: issue7213 + issue2320: Cause a DeprecationWarning if the close_fds argument is not passed to subprocess.Popen as the default value will be changing

in

...

...

a future Python to the safer and more often desired value of True.

That doesn't seem to be a good idea under Windows, is it?

(“Note that on Windows, you cannot set *close_fds* to true and also redirect the standard handles by setting *stdin*, *stdout* or *stderr*.”)

Regardless of what platform you are on I think the warning makes sense, it was just worded too strongly. It is asking people to be explicit with close_fds. Explicitly setting close_fds=False when that is desired is good. I modified the docs and warning message to just say that the default close_fds behavior will change in the future without specifying exactly what it will be. It could make sense for the default to be a soft-True on windows that changes behavior if any of the std handles are set if that matches what users expect and find easiest. We may want to reconsider its entire future in the face of the new pass_fds parameter, etc. Those are 3.3 decisions.

This sounds like omitting the close_fds parameter is now considered ill-advised, if not outright wrong. That's silly - I certainly never specify close_fds, expecting the module to do the correct thing if I don't know/care enough to say. I use Popen as a convenience function, so ignoring low-level details is the whole point in my opinion (and close_fds *is* a low-level detail for what I do, on Windows).

At the very least, the whole of the section "Replacing Older Functions with the subprocess Module" (with a couple of small exceptions) is in need of updating, as it is recommending bad practice (not specifying close_fds) at the moment...

OK, I'm exaggerating a touch here. But can someone point me at the discussion of this change? Or if there hasn't been one, can we have one (i.e., to start the ball rolling, can someone justify the change, please).

Paul.

Making the change was intended to force the discussion. I'm glad that worked. :)

I don't like the thought of requiring people to specify close_fds either but the default is a bad one (see http://bugs.python.org/issue7213 and http://bugs.python.org/issue2320) so we should change it.

The real question is how should we go about doing the change? This warning asking people to always specify close_fds=True is heavy handed. Other places in the stdlib and docs no doubt still need to be updated to reflect it, etc.

Options that seem worthy of debate:

A) The DeprecationWarning that exists in py3k as of today.

B) Remove the DeprecationWarning, simply document that people should be explicit about it if they care at all and that the default will change in 3.3.

C) Never change close_fds behavior. we're stuck with it for life.

D) Deprecate close_fds so that it becomes a legacy no-op. the new pass_fds flag could evolve into this. this will break existing code that depends on close_fds one way or another.

I'm in 100% agreement that forcing people to pass close_fds in makes the API less friendly (granted, people already find it unfriendly but why make it worse?).

Option B seems the most friendly to me.

+1 from me. People who don't care about the 'close_fds' behavior one way or the other shouldn't get a warning which only helps the tiny (I assert) minority who a) *do* care but b) don't pass the flag explicitly. Tres.. - -- =================================================================== Tres Seaver +1 540-429-0999 tseaver@palladion.com Palladion Software "Excellence by Design" http://palladion.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkz7iU4ACgkQ+gerLs4ltQ4SJgCfePUImv5OSHzzZ4QJvzUz1oYJ LhAAoKRut3AfGkS23hghQx9pd3D0WF3p =y8hn -----END PGP SIGNATURE-----

On Sun, 5 Dec 2010 11:08:43 -0800 "Gregory P. Smith" wrote:

Sleeping on the issue some more and pondering it...

Is there any _good_ reason not to just make the close_fds default change in 3.2 today? No warning (since they're never seen by most people anyways). Document it in Misc/NEWS and whatsnew.rst.

Yes, it will break many scripts under Windows. That's the core of the issue, really. I agree under Unix it makes sense to change the default. Regards Antoine.