Last Comment Bug 782866 - Pymake: commands that don't use shells don't honour exported PATH on Win32
: Pymake: commands that don't use shells don't honour exported PATH on Win32
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on:
Blocks: 593585
  Show dependency treegraph
 
Reported: 2012-08-14 19:03 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2012-08-23 08:32 PDT (History)
4 users (show)
sid.bugzilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (32.25 KB, patch)
2012-08-15 06:06 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Review
patch with fixed executable bit (32.54 KB, patch)
2012-08-21 14:03 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Review
remove threadpool, and make sure we're in a different process (33.86 KB, patch)
2012-08-21 18:02 PDT, Siddharth Agarwal [:sid0] (inactive)
gps: review+
Details | Diff | Review

Description Siddharth Agarwal [:sid0] (inactive) 2012-08-14 19:03:05 PDT
See http://bugs.python.org/issue15451
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2012-08-14 19:11:28 PDT
I think the right thing to do here will be to special-case PATH. Gah...
Comment 2 Siddharth Agarwal [:sid0] (inactive) 2012-08-14 19:24:15 PDT
See also: http://bugs.python.org/issue8557
Comment 3 Mike Hommey [:glandium] 2012-08-14 23:30:25 PDT
all non native commands should be running with sh -c on msys, cf. http://mxr.mozilla.org/mozilla-central/source/build/pymake/pymake/process.py#86
Comment 4 Siddharth Agarwal [:sid0] (inactive) 2012-08-15 01:52:23 PDT
For a simple command, e.g. |cl|, shellreason will be None.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2012-08-15 05:19:51 PDT
Right, that was one of the big Pymake wins (not spawning extraneous shells). How did our build ever work if this is broken? Things like "rm foo" would presumably not work.
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2012-08-15 05:22:04 PDT
Sorry, the title was a bit unclear: they do honour the parent's path normally. They just don't honour the PATH variable if it's exported from within the makefile, as we have in our Win32 mozconfigs.

I have a working patch -- struggling to write a test right now.
Comment 7 Siddharth Agarwal [:sid0] (inactive) 2012-08-15 06:06:38 PDT
Created attachment 652083 [details] [diff] [review]
patch v1

I believe this should fix things up. I'm running a test build on my build slave to make sure it does.
Comment 8 Siddharth Agarwal [:sid0] (inactive) 2012-08-15 09:47:58 PDT
Yes, the build works now.
Comment 9 Gregory Szorc [:gps] 2012-08-15 09:52:28 PDT
(In reply to Ted Mielczarek [:ted] from comment #5)
> Right, that was one of the big Pymake wins (not spawning extraneous shells).
> How did our build ever work if this is broken? Things like "rm foo" would
> presumably not work.

I would throw out that I believe a proper build system deduces absolute paths during a "configure" phase and all paths in the build phase (even for things that are not binaries) are absolute paths.

I threw in *all* paths because we have a lot of code today that works on relative paths. For example, the include paths are relative. In the case of includes, those relative paths get written in the auto-generated makefile dependency files, which means that that .mk file can only be evaluated from a specific directory. This prevents non-recursive evaluation. Doh.
Comment 10 Siddharth Agarwal [:sid0] (inactive) 2012-08-21 14:03:31 PDT
Created attachment 653937 [details] [diff] [review]
patch with fixed executable bit

With this fix the test works on Linux as well.
Comment 11 Gregory Szorc [:gps] 2012-08-21 16:08:02 PDT
Comment on attachment 653937 [details] [diff] [review]
patch with fixed executable bit

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

The hack, it hurts.

This is effectively r+. I just want answers to my questions first.

::: pymake/process.py
@@ +172,3 @@
>          try:
> +            if self.env is not None and self.env.has_key('PATH'):
> +                os.environ['PATH'] = self.env['PATH']

I understand modifying os.environ is safe given the current execution model. But, it makes me uneasy.

To help me rest better, how do you feel about setting a module-level flag when running and asserting in run() that it isn't set? i.e. if we ever change the execution model of pymake, I want this to die loudly.

::: tests/subprocess-path.mk
@@ +1,4 @@
> +#T gmake skip
> +#T grep-for: "2f7cdd0b-7277-48c1-beaf-56cb0dbacb24"
> +
> +ifdef __WIN32__

Where does this come from?

@@ +22,5 @@
> +#
> +# Q. Why use an exe and not a batch file?
> +# A. The use cases here were all exe files without the extension. Batch file
> +#    lookup has broken semantics if the .bat extension isn't passed.
> +#  

Whitespace.
Comment 12 Siddharth Agarwal [:sid0] (inactive) 2012-08-21 18:02:13 PDT
Created attachment 654042 [details] [diff] [review]
remove threadpool, and make sure we're in a different process

Good catch! This patch means that now all commands are run through child processes.

__WIN32__ is defined in runtests.py.
Comment 13 Ted Mielczarek [:ted.mielczarek] 2012-08-22 06:52:24 PDT
So um, you mean you regressed bug 602506?
Comment 14 Siddharth Agarwal [:sid0] (inactive) 2012-08-22 06:59:09 PDT
Hmm, I guess so. However I did do a full build on a build slave and it took about the same time, give or take a few seconds.

I don't see how this bug can be reconciled with bug 602506 :(
Comment 15 Siddharth Agarwal [:sid0] (inactive) 2012-08-22 08:07:04 PDT
On a build slave, a -j4 build with same-process Popen took 32:59, while a -j4 build with different-process Popen took 33:14. I don't think it's a big concern any more.
Comment 16 Gregory Szorc [:gps] 2012-08-22 20:45:38 PDT
Comment on attachment 654042 [details] [diff] [review]
remove threadpool, and make sure we're in a different process

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

The removal of the multiprocessing.dummy foo scared me. But, apparently it doesn't make a difference and Ted's OK with it. So, I'm OK with it.

Note You need to log in before you can comment on or make changes to this bug.