950894 - [mozprocess] TypeError under windows evironment in cpython 2.7.6

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jarek Śmiejczak [:jotes]]

For some reason, mozprocess redefines _execute_child method under windows environment.
Unfortunately after changes in cpython 2.7.6:
http://hg.python.org/cpython/rev/43749cb6bdbd
ProcessHandler.run() raises TypeError Exception:
TypeError: _execute_child() takes exactly 17 arguments (18 given)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Arg, that change has not been implemented in a way that it would allow us to have it easily updated in terms of backward compatibility. :/

http://hg.python.org/cpython/rev/43749cb6bdbd#l1.13

Jarek, can you please tell us the whole exception details?

Comment 2

[Jarek Śmiejczak [:jotes]]

:whimboo
Sure:
https://gist.github.com/jarekps/3877791a05caedf064de

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks but lets get it as comment on the bug so it is queryable:

Traceback (most recent call last):
File "build.py", line 52, in <module>
compile_executables()
File "build.py", line 39, in compile_executables
process.run()
File "c:\python27\lib\site-packages\mozprocess\processhandler.py", line 642, in run
self.proc = self.Process([self.cmd] + self.args, **args)
File "c:\python27\lib\site-packages\mozprocess\processhandler.py", line 96, in __init__
universal_newlines, startupinfo, creationflags)
File "c:\python27\lib\subprocess.py", line 709, in __init__
errread, errwrite)
TypeError: _execute_child() takes exactly 17 arguments (18 given)

Hm, so we specify more parameters as required. I wonder which one is causing us the problem.

Comment 4

[uFFFD]

This one is the same as bug 958609

When python is calling self._execute_child with 18 arguments from subprocess.Popen.__init__ in python 2.7.6's lib\subprocess.py,
it's actually calling Process._execute_child in our mozbase\mozprocess\mozprocess\processhandler.py, which only takes 17 arguments
the extra argument is to_close, between shell and p2cread
I think we'd better change definition of Process._execute_child like this for backward compatibility:

def _execute_child(self, *args_tuple):
    if sys.hexversion < 0x02070600: # prior to 2.7.6
        (......) = args_tuple
    else: # 2.7.6 and later
        (..., shell, to_close, p2cread,...) = args_tuple

Comment 5

[Jarek Śmiejczak [:jotes]]

:uFFFD

Yes, you're correct (it was described a little non-obviously in links above, sorry for that).
Feel free to provide patch and give :ahal or :whimboo to review. I can also provide initial review/feedback if you're interested.

Comment 6

[uFFFD]

Attachment: patch for processhandler.py in cpython 2.7.6 under windows

Comment 7

[Jarek Śmiejczak [:jotes]]

Comment on attachment 8363773 [details] [diff] [review]
patch for processhandler.py in cpython 2.7.6 under windows

Review of attachment 8363773 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozprocess/mozprocess/processhandler.py
@@ +157,5 @@
>  
>          if isWin:
>              # Redefine the execute child so that we can track process groups
> +            def _execute_child(self, *args_tuple):
> +                # workaround for bug 950894

I'm not sure about our time limits here (for this blocker) -> maybe it would be easy to implement?
If that's not possible we should add next bug to handle this parameter properly (or ignore as a result of investigation).

@@ +164,5 @@
> +                     cwd, env, universal_newlines, startupinfo,
> +                     creationflags, shell,
> +                     p2cread, p2cwrite,
> +                     c2pread, c2pwrite,
> +                     errread, errwrite) = args_tuple

I would add to_close = None, for future implementations to avoid NameError between code versions.

Comment 8

[Jarek Śmiejczak [:jotes]]

:uFFFD
Hi, i've done a little review :-) There's only one thing which would be important for me (this possible NameError), rest looks good enough.
However, you must wait for Henrik's opinion.

Comment 9

[uFFFD]

Hi, Jarek Śmiejczak
Thank you for the review

(In reply to Jarek Śmiejczak from comment #7)
> I would add to_close = None, for future implementations to avoid NameError
> between code versions.
to_close is used in try-except's finally block in Popen._execute_child from subprocess.py of CPython
but we don't have that try-except block in Process._execute_child from our processhandler.py

> I'm not sure about our time limits here (for this blocker) -> maybe it would
> be easy to implement?
> If that's not possible we should add next bug to handle this parameter
> properly (or ignore as a result of investigation).
I'm not sure if we should wrap winprocess.CreateProcess in try-except block in our Process._execute_child
and close those handles in to_close in finally block like CPython 2.7.6 does
This is just one quick workaround for mozprocess to run under CPython 2.7.6 on windows
Sorry, but I'm afraid we need more investigating :(

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8363773 [details] [diff] [review]
patch for processhandler.py in cpython 2.7.6 under windows

Review of attachment 8363773 [details] [diff] [review]:
-----------------------------------------------------------------

I would better ask Joel as reviewer here. He is deeper into mozprocess as I am. Especially for code which gets run on Windows.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8363773 [details] [diff] [review]
patch for processhandler.py in cpython 2.7.6 under windows

Review of attachment 8363773 [details] [diff] [review]:
-----------------------------------------------------------------

this seems like a fine fix.  We will always have these issues to workaround, please add a reference to to_close in the <2.7.6 case so we have the variable around.

::: mozprocess/mozprocess/processhandler.py
@@ +164,5 @@
> +                     cwd, env, universal_newlines, startupinfo,
> +                     creationflags, shell,
> +                     p2cread, p2cwrite,
> +                     c2pread, c2pwrite,
> +                     errread, errwrite) = args_tuple

++

Comment 12

[uFFFD]

Attachment: add a reference to to_close in the <2.7.6 case

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

I was looking for references to to_close or additional arguments and I haven't seen them.  Can you point me to references of subprocess before/after 2.7.6 that has the additional argument to execute_child?

Comment 14

[uFFFD]

Sorry

As Jarek and Henrik said, it's Issue #18851
http://hg.python.org/cpython/rev/43749cb6bdbd

It can be found in changelog of 2.7.6
http://hg.python.org/cpython/raw-file/99d03261c1ba/Misc/NEWS

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

so this assumes we have updated python 2.7.6 since August 2013 on our machines.  I think this is a false assumption- although I would bet some of our machines have the newer version.

Thanks for linking to this change.  How can  we assure that an older version of python 2.7.6+ will work with this code?

this patch is looking good so far.

Comment 16

[uFFFD]

to_close is a set that may contain duplicated values of p2cread, p2cwrite, c2pwrite, c2pread, errwrite and errread
http://hg.python.org/cpython/rev/43749cb6bdbd#l1.45

Popen._execute_child only uses it in a finally clause to close p2cread, c2pwrite and errwrite
http://hg.python.org/cpython/rev/43749cb6bdbd#l1.103
http://hg.python.org/cpython/rev/43749cb6bdbd#l1.115
Our Process._execute_child has p2cread, c2pwrite and errwrite explicitly closed if no exception is raised

Popen.__init__, the caller of Popen._execute_child or our Process._execute_child, only uses it in an except clause to close those handles
http://hg.python.org/cpython/rev/43749cb6bdbd#l1.31

I think older versions of python 2.7.6 should work as long as we don't use to_close in our _execute_child
We can use p2cread, p2cwrite, c2pwrite, c2pread, errwrite and errread directly instead,
or generate that set in sys.hexversion < 0x02070600 case and access it later

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8364352 [details] [diff] [review]
add a reference to to_close in the <2.7.6 case

lets get this in.  I think we have accounted for the difference as best as we can.  Thanks for making this happen!

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So who is actually going to land this Joel? The assignee doesn't have the permissions for doing that.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

I think with the checkin-needed flag it will get picked up within the next day!

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

But this has to be landed on github and not mozilla-central. Would you mind landing it, given that you reviewed it?

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

umm, mozbase is on mozilla-central now, the github version is not the master.  I don't mind landing it on github, but I need confirmation that we a really developing on there.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No, it is not! At least not as long bug 949600 has been fixed.

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for the clarification.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

https://github.com/mozilla/mozbase/commit/835d824f09b7458fd887da26865cdf822cd9c8f1