Hi Everyone, I now have an application-level implementation of os.wait() complete. It's built on top of os.waitpid(), but according to the documentation I've found, the behavior should be the same. My test might be a bit objectionable as well, but I wanted to use a non-zero return value. I also modified test_popen() as it wasn't waiting for its children, and broke my test_os_wait(), since it was waiting for the child processes created by test_popen(). In the future it would be desirable to implement os.wait() in terms of the c wait() function in ll_os.py but currently (at least on Ubuntu 9.10) the c wait() function takes a pointer to a transparent struct, rather than an int, and that seems to prevent my interpreter level version from being built. I've attached both versions, but the interpreter level version will not translate. cheers, Dan P.S. This is my first patch, so it's probably worth giving it at least a once over...
Hi Dan, On Sat, Nov 21, 2009 at 12:00:38PM -0800, Dan Roberts wrote:
I now have an application-level implementation of os.wait() complete. It's built on top of os.waitpid(), but according to the documentation I've found, the behavior should be the same.
Thank you !
+ def wait(): + import os + return os.waitpid(-1, 0)
It's more a general issue with app_posix.py, but I think that it should avoid these costly "import os" inside application-level functions. You already have the posix module imported (see the start of app_posix.py), so you can return posix.waitpid(-1, 0). The same comment applies to other places too, e.g. popenfile.close(). A bientot, Armin.
Hi Armin, Thanks for the reply, I've attached a new version of my patch. At first glance it would appear I've done little to address your concerns, however there's good reason for this. As it turns out, our implementation of popen() depends on execvp(), which is only implemented in the os module (specifically, the one borrowed from CPython). I did write an interpreter level version of execvp(), which would have allowed me to remove the imports of os from popen(), however I don't think it's wise to replace our existing application level implementation with my interpreter level implementation. Because of that, I can't remove the os dependency from popen(). For consistency, i left os in both popen() and popenfile because they directly interact. I did, however, modify wait() to use posix instead. Even though I didn't eliminate the os imports, my test *did* improve a bit from the old version, so this iteration isn't totally useless. :-) All posix tests are passing again, including the new os.wait() test. I'll be happy to address any other concerns you, or anyone else have, and if you'd like to remove those os imports, I'd like to talk to you about how to best approach that. Cheers, Dan On Sun, 2009-11-22 at 12:29 +0100, Armin Rigo wrote:
Hi Dan,
On Sat, Nov 21, 2009 at 12:00:38PM -0800, Dan Roberts wrote:
I now have an application-level implementation of os.wait() complete. It's built on top of os.waitpid(), but according to the documentation I've found, the behavior should be the same.
Thank you !
+ def wait(): + import os + return os.waitpid(-1, 0)
It's more a general issue with app_posix.py, but I think that it should avoid these costly "import os" inside application-level functions. You already have the posix module imported (see the start of app_posix.py), so you can return posix.waitpid(-1, 0). The same comment applies to other places too, e.g. popenfile.close().
A bientot,
Armin.
Hi Dan, On Wed, Nov 25, 2009 at 12:27:27AM -0800, Dan Roberts wrote:
Thanks for the reply, I've attached a new version of my patch. At first glance it would appear I've done little to address your concerns, however there's good reason for this. As it turns out, our implementation of popen() depends on execvp(), which is only implemented in the os module (specifically, the one borrowed from CPython).
Ah, ok. I see now why there is 'import os' there. So that means that your patch with 'posix.waitpid' instead of 'os.waitpid' is fine. A bientot, Armin.
Commited On Thu, Nov 26, 2009 at 11:24 AM, Armin Rigo <arigo@tunes.org> wrote:
Hi Dan,
On Wed, Nov 25, 2009 at 12:27:27AM -0800, Dan Roberts wrote:
Thanks for the reply, I've attached a new version of my patch. At first glance it would appear I've done little to address your concerns, however there's good reason for this. As it turns out, our implementation of popen() depends on execvp(), which is only implemented in the os module (specifically, the one borrowed from CPython).
Ah, ok. I see now why there is 'import os' there. So that means that your patch with 'posix.waitpid' instead of 'os.waitpid' is fine.
A bientot,
Armin. _______________________________________________ pypy-dev@codespeak.net http://codespeak.net/mailman/listinfo/pypy-dev
participants (3)
-
Armin Rigo -
Dan Roberts -
Maciej Fijalkowski