After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 583455 - [win32 fixes] jhbuild uses subprocess.Popen incompatibly with win32
[win32 fixes] jhbuild uses subprocess.Popen incompatibly with win32
Status: RESOLVED FIXED
Product: jhbuild
Classification: Infrastructure
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Jhbuild maintainers
Jhbuild QA
Depends on:
Blocks:
 
 
Reported: 2009-05-21 14:41 UTC by Sam Thursfield
Modified: 2009-06-04 09:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fixes win32 subprocess issues (7.77 KB, patch)
2009-05-21 14:41 UTC, Sam Thursfield
needs-work Details | Review
1st revision (6.10 KB, patch)
2009-05-25 13:50 UTC, Sam Thursfield
none Details | Review
revised^2 patch (12.90 KB, patch)
2009-05-29 14:25 UTC, Sam Thursfield
none Details | Review
revised^3 patch (11.67 KB, patch)
2009-05-29 14:53 UTC, Sam Thursfield
reviewed Details | Review
final (i hope) patch (11.70 KB, patch)
2009-06-03 23:29 UTC, Sam Thursfield
none Details | Review

Description Sam Thursfield 2009-05-21 14:41:20 UTC
Please describe the problem:
Attached is a patch which:

 * Moves subprocess.Popen calls into cmds.get_subprocess, and fixes get_output on Win32.
 * Adds jhprocess to utils/cmds.py: this class (mostly by John Stowers) fixes path separators, shell execution, certain environment prefixes and output piping/capture including emulation of close_fds.


Steps to reproduce:


Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Sam Thursfield 2009-05-21 14:41:46 UTC
Created attachment 135106 [details] [review]
fixes win32 subprocess issues
Comment 2 Frederic Peters 2009-05-21 18:08:49 UTC
This definitely needs work, for example:

 - splitting commands on spaces to get a list will break in many cases (the
   most obvious one is paths with spaces).

 - HACK_PREFIX = 'C:\\msys\\' (already marked with FIXME)

 - HACK_ENV_VARS = ['PKG_CONFIG_PATH','CFLAGS','LDFLAGS'], why couldn't those
   environment variables be set in a form acceptable by Windows in config.py,
   instead of doing the job everytime a command needs to be run?

 - other buildscripts also have a execute() method
Comment 3 Frederic Peters 2009-05-24 08:13:18 UTC
Thinking about it today, I wonder if this part couldn't be made in a win32kludge.py module that would monkeypatch subprocess.Popen, not changing a single line to frontends/*.py.

Would you adapt the patch that way?
Comment 4 Sam Thursfield 2009-05-25 13:50:14 UTC
Created attachment 135321 [details] [review]
1st revision

Good idea. I've reimplemented from scratch - this misses out various parts of the original patch which so far haven't proved necessary. However there's some weird errors I still need to investigate so I might need to post some more patches.

However, it is possible to build some of the simpler modules with just git HEAD and this patch (and various parts of MSYS :)
Comment 5 Sam Thursfield 2009-05-26 17:41:26 UTC
This still needs some more work actually. I'll post a 2nd revised patch soon.
Comment 6 Sam Thursfield 2009-05-29 14:25:00 UTC
Created attachment 135552 [details] [review]
revised^2 patch

With this patch version, one of the end to end autoools tests passes!
Comment 7 Sam Thursfield 2009-05-29 14:53:40 UTC
Created attachment 135558 [details] [review]
revised^3 patch

Sorry for the patch spam. Fixed 2 whitespace errors I hadn't noticed, and some other dumb mistakes, and fixed accidentally running .exes through sh needlessly.
Comment 8 Frederic Peters 2009-06-02 15:53:07 UTC
+        if envvar in [ 'PATH' ]:

You could just do if envvar == 'PATH':

In tests.py:

-        self.assertEquals(stdout, 'Hello world (distutils)\n')
+        self.assertEquals(stdout, 'Hello world (distutils)'+os.linesep)

I'd prefer
+        self.assertEquals(stdout.strip(), 'Hello world (distutils)')

(there are several occurences)

Otherwise it looks good, thanks for working on this, please attach a fixed patch and I'll commit it.
Comment 9 Sam Thursfield 2009-06-03 23:29:42 UTC
Created attachment 135911 [details] [review]
final (i hope) patch

all done hopefully! thanks for your support!
Comment 10 Frederic Peters 2009-06-04 09:26:19 UTC
I reformatted some long comments to fit in 80 columns, and removed the print statement that was subprocess_win32.py.

commit 16bb6ae3761592042868f11bd4ff359667e8df71
Author: Sam Thursfield <ssssam@gmail.com>
Date:   Thu Jun 4 11:21:26 2009 +0200

    [win32] monkeypatch subprocess.Popen on Microsoft Windows (GNOME bug 583455)