|
33.86 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
|
(Assignee) | |
Comment 1•5 years ago
|
||
I think the right thing to do here will be to special-case PATH. Gah...
|
|
(Assignee) | |
Comment 2•5 years ago
|
||
See also: http://bugs.python.org/issue8557
|
||
Comment 3•5 years ago
|
||
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
|
|
(Assignee) | |
Comment 4•5 years ago
|
||
For a simple command, e.g. |cl|, shellreason will be None.
|
||
Comment 5•5 years ago
|
||
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.
|
|
(Assignee) | |
Comment 6•5 years ago
|
||
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.
|
|
(Assignee) | |
Comment 7•5 years ago
|
||
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.
|
|
(Assignee) | |
Comment 8•5 years ago
|
||
Yes, the build works now.
|
||
Comment 9•5 years ago
|
||
(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.
|
|
(Assignee) | |
Comment 10•5 years ago
|
||
Created attachment 653937 [details] [diff] [review] patch with fixed executable bit With this fix the test works on Linux as well.
|
|
||
Comment 11•5 years ago
|
||
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.
|
|
(Assignee) | |
Comment 12•5 years ago
|
||
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•5 years ago
|
||
So um, you mean you regressed bug 602506?
|
|
(Assignee) | |
Comment 14•5 years ago
|
||
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 :(
|
|
(Assignee) | |
Comment 15•5 years ago
|
||
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•5 years ago
|
||
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.
|
|
(Assignee) | |
Comment 17•5 years ago
|
||
http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/5d2775d6a2d5 http://hg.mozilla.org/mozilla-central/rev/21a815b9f4c7
Description
•