Closed
Bug 680636
Opened 13 years ago
Closed 12 years ago
Execute nsinstall.py natively under PyMake
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla16
People
(Reporter: gps, Assigned: rain1)
References
(Depends on 1 open bug)
Details
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
rain1
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rain1
:
review+
|
Details | Diff | Splinter Review |
PyMake has the ability to call out to Python modules/methods natively instead of invoking separate Python interpreters. We should define $(NSINSTALL) in config.mk to do this when running under PyMake. FWIW, $(NSINSTALL) is executed ~1650 times during a fresh build of the browser profile. As we know, processes on Windows are expensive (compared to other OS's), so a savings of 1650 extra processes should help bring down Windows build times. By how much, I have no clue.
Reporter | ||
Comment 1•13 years ago
|
||
This patches config.mk to run nsinstall.py natively under PyMake. I've tested it on my local Linux VM and it seems to work fine in both make and PyMake. The patch includes a one-liner to nsinstall.py to change a 'sys.exit()' to 'return' from within the method. (The method should always return, never exit.) I have not yet performed a Try build. That should definitely be done before this lands anywhere. I created the patch from Git, so that's why the format is screwy. I can repost in Mercurial format if requested.
Attachment #554613 -
Flags: review?(khuey)
Comment 2•13 years ago
|
||
Comment on attachment 554613 [details] [diff] [review] Execute nsinstall.py natively under PyMake >+# We prefer the PyMake native invokation about all others because it will not >+# spawn a new process. >+ifdef .PYMAKE You probably meant "pymake invocation above all others". IMO we don't need to explain why pymake native commands are preferred. The patch looks good to me, but I'll defer to khuey for definitive review.
Reporter | ||
Comment 3•13 years ago
|
||
(In reply to Mitchell Field [:Mitch] from comment #2) > You probably meant "pymake invocation above all others". Coming from PyMake land, "native" means "native Python" (see pymake/data.py:getcommandsforrule() ). But, we aren't inside PyMake here, so I can see how the terminology is confusing. I'll change it on the next patch or for the commit if this one is r-conditional.
Comment 4•13 years ago
|
||
I wouldn't bother with the comment at all, honestly. We haven't had any comments like that elsewhere.
Comment 5•13 years ago
|
||
Oh, I think that the reason I hadn't proposed this yet is that nsinstall.py doesn't support creating symlinks currently. Not that anyone is going to use pymake on Linux/OS X, but it would be nice if building with pymake didn't have subtle differences like that.
Comment on attachment 554613 [details] [diff] [review] Execute nsinstall.py natively under PyMake I think we can live with the difference for the moment, but please find or file a followup on finishing nsinstall.py's implementation.
Attachment #554613 -
Flags: review?(khuey) → review+
Also, in the future, 8 lines of context would be better.
Reporter | ||
Comment 8•13 years ago
|
||
The content is identical to the previous patch except it is formatted properly and has the comment removed per comment from ted.
Attachment #554613 -
Attachment is obsolete: true
Reporter | ||
Comment 9•13 years ago
|
||
This is technically ready for committing. However, I have yet to perform a Try build. If you want me to perform a Try build, just ping me here or on IRC. The only reason I haven't done a Try build yet is I haven't performed a Try build before and I want to grok the documentation before I perform one.
Keywords: checkin-needed
Try doesn't run pymake so you're not going to learn that much.
Comment 11•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/e10579e136f1
Assignee: nobody → gps
Status: NEW → ASSIGNED
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
Comment 12•13 years ago
|
||
Try run for 6f63ee0f78d4 is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=6f63ee0f78d4 Results (out of 5 total builds): exception: 5 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-6f63ee0f78d4
Comment 13•13 years ago
|
||
This patch causes my local pymake build to break: http://pastebin.com/fC5dCuRm Mozconfig used: http://pastebin.com/dVbQNp2m Build script used: http://pastebin.com/S0wMD867 (ie autoconf-2.13 then configure rather than client.mk) My directory structure is such that objdir isn't inside srcdir, don't know if that would make any difference. (ie: c:\mozilla\repos\inbound\ and c:\mozilla\repos\obj-inbound\). Using VC2010, Win SDK 7.0A, MozillaBuild 1.6rc.
Reporter | ||
Comment 14•13 years ago
|
||
In response to comment #13, it appears the problem is $(NSINSTALL) is being used within a larger script. It is now obvious that after this change, $(NSINSTALL) would not be safe to use outside of single-line commands because the shell would not know how to invoke using the Python syntax. So, to properly implement this patch, we'll need to audit the code base for usages of $(NSINSTALL) inside command/shell blocks. This change isn't as trivial as I hoped. Ugh.
Comment 15•13 years ago
|
||
Backed out of inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/408b90991d17
Target Milestone: mozilla9 → ---
Ugh, that's pretty nasty :-/
Reporter | ||
Comment 17•13 years ago
|
||
So, what do we do for the PyMake built-in/native-Python commands (like rm, mkdir, pythonpathy, etc)? Do we just not utilize them in multiline recipes that are invoked under a single shell? If so, perhaps a useful feature of PyMake would be to error upon detection of these commands.
Comment 18•13 years ago
|
||
Yeah, we shouldn't be trying to use them in those situations. It's an unfortunate case I hadn't really thought of when I implemented native command support. Ideally we would just rewrite the Makefiles to remove complex shell invocations, replacing them with simpler Makefile constructs or calls to Python scripts containing the logic.
Assignee | ||
Comment 19•13 years ago
|
||
This patch goes on top of the one in bug 757252.
Assignee: gps → sagarwal
Attachment #554913 -
Attachment is obsolete: true
Attachment #628064 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 21•12 years ago
|
||
I pushed this to try and didn't see any issues.
Attachment #628064 -
Attachment is obsolete: true
Attachment #628064 -
Flags: review?(ted.mielczarek)
Attachment #633535 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #628066 -
Attachment is obsolete: true
Attachment #628066 -
Flags: review?(ted.mielczarek)
Attachment #633536 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 23•12 years ago
|
||
Attachment #633535 -
Attachment is obsolete: true
Attachment #633535 -
Flags: review?(ted.mielczarek)
Attachment #635871 -
Flags: review?(ted.mielczarek)
Comment 24•12 years ago
|
||
Comment on attachment 633536 [details] [diff] [review] now with proper unicode support (c-c) Review of attachment 633536 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/config.mk @@ +575,5 @@ > PWD := $(CURDIR) > endif > > NSINSTALL_PY := $(PYTHON) $(call core_abspath,$(MOZILLA_SRCDIR)/config/nsinstall.py) > +# For Pymake, whereever we use nsinstall.py we're also going to try to make it "wherever" @@ +611,5 @@ > > endif # WINNT/OS2 > > +# The default for install_dist is simply INSTALL > +install_dist ?= $(INSTALL) $(1) install_dist seems like a confusing name since it doesn't actually install to DIST. I might just call this "install_cmd" or something.
Attachment #633536 -
Flags: review?(ted.mielczarek) → review+
Comment 25•12 years ago
|
||
Comment on attachment 635871 [details] [diff] [review] patch updated to fix bitrot (m-c) Review of attachment 635871 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/config.mk @@ +637,5 @@ > +# For Pymake, whereever we use nsinstall.py we're also going to try to make it > +# a native command where possible. Since native commands can't be used outside > +# of single-line commands, we continue to provide INSTALL for general use. > +# Single-line commands should be switched over to install_dist. > +NSINSTALL_NATIVECMD := %nsinstall nsinstall_native Kind of a bummer that you had to name it "nsinstall_native" instead of just using "nsinstall". @@ +657,5 @@ > > ifeq (,$(CROSS_COMPILE)$(filter-out WINNT OS2, $(OS_ARCH))) > +INSTALL = $(NSINSTALL) -t > +ifdef .PYMAKE > +install_dist = $(NSINSTALL_NATIVECMD) -t $(1) Same comment as the other patch.
Attachment #635871 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 26•12 years ago
|
||
> Kind of a bummer that you had to name it "nsinstall_native" instead of just using "nsinstall".
Turns out switching it over isn't a problem at all.
Attachment #635871 -
Attachment is obsolete: true
Attachment #638097 -
Flags: review+
Assignee | ||
Comment 27•12 years ago
|
||
The two patches are blocked on the ones in bug 757252. I'll check them in together.
Attachment #633536 -
Attachment is obsolete: true
Attachment #638098 -
Flags: review+
Assignee | ||
Comment 28•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/88aaf6c529b9 http://hg.mozilla.org/comm-central/rev/a013d9521c46
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•