Last Comment Bug 680636 - Execute nsinstall.py natively under PyMake
: Execute nsinstall.py natively under PyMake
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on: 680968 757252 770558 770702 772186
Blocks: 585010
  Show dependency treegraph
 
Reported: 2011-08-19 23:18 PDT by Gregory Szorc [:gps]
Modified: 2012-07-11 04:33 PDT (History)
6 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Execute nsinstall.py natively under PyMake (2.41 KB, patch)
2011-08-19 23:30 PDT, Gregory Szorc [:gps]
khuey: review+
Details | Diff | Review
Execute nsinstall.py as Python module under PyMake (3.14 KB, patch)
2011-08-22 11:18 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Review
handle a large subset of nsinstall invocations (m-c) (14.98 KB, patch)
2012-05-29 12:41 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Review
handle a large subset of nsinstall invocations (c-c) (10.71 KB, patch)
2012-05-29 12:42 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Review
now with proper unicode support (m-c) (20.67 KB, patch)
2012-06-15 08:17 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Review
now with proper unicode support (c-c) (10.80 KB, patch)
2012-06-15 08:17 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review+
Details | Diff | Review
patch updated to fix bitrot (m-c) (21.48 KB, patch)
2012-06-22 12:48 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review+
Details | Diff | Review
what I'm going to check in (m-c) (14.56 KB, patch)
2012-06-30 04:11 PDT, Siddharth Agarwal [:sid0] (inactive)
sid.bugzilla: review+
Details | Diff | Review
what I'm going to check in (c-c) (10.76 KB, patch)
2012-06-30 04:12 PDT, Siddharth Agarwal [:sid0] (inactive)
sid.bugzilla: review+
Details | Diff | Review

Description Gregory Szorc [:gps] 2011-08-19 23:18:26 PDT
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.
Comment 1 Gregory Szorc [:gps] 2011-08-19 23:30:02 PDT
Created attachment 554613 [details] [diff] [review]
Execute nsinstall.py natively under PyMake

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.
Comment 2 Mitchell Field [:Mitch] 2011-08-20 00:01:28 PDT
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.
Comment 3 Gregory Szorc [:gps] 2011-08-20 00:21:14 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2011-08-20 05:18:58 PDT
I wouldn't bother with the comment at all, honestly. We haven't had any comments like that elsewhere.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2011-08-20 05:22:39 PDT
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 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-22 08:38:03 PDT
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.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-22 08:42:08 PDT
Also, in the future, 8 lines of context would be better.
Comment 8 Gregory Szorc [:gps] 2011-08-22 11:18:29 PDT
Created attachment 554913 [details] [diff] [review]
Execute nsinstall.py as Python module under PyMake

The content is identical to the previous patch except it is formatted properly and has the comment removed per comment from ted.
Comment 9 Gregory Szorc [:gps] 2011-08-22 11:20:32 PDT
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.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-22 11:23:09 PDT
Try doesn't run pymake so you're not going to learn that much.
Comment 12 Mozilla RelEng Bot 2011-08-22 11:40:43 PDT
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 Ed Morley [:emorley] 2011-08-22 13:50:50 PDT
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.
Comment 14 Gregory Szorc [:gps] 2011-08-22 14:13:20 PDT
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 Ed Morley [:emorley] 2011-08-22 14:30:09 PDT
Backed out of inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/408b90991d17
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-22 15:06:21 PDT
Ugh, that's pretty nasty :-/
Comment 17 Gregory Szorc [:gps] 2011-08-22 15:18:11 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-08-23 04:39:46 PDT
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.
Comment 19 Siddharth Agarwal [:sid0] (inactive) 2012-05-29 12:41:54 PDT
Created attachment 628064 [details] [diff] [review]
handle a large subset of nsinstall invocations (m-c)

This patch goes on top of the one in bug 757252.
Comment 20 Siddharth Agarwal [:sid0] (inactive) 2012-05-29 12:42:33 PDT
Created attachment 628066 [details] [diff] [review]
handle a large subset of nsinstall invocations (c-c)
Comment 21 Siddharth Agarwal [:sid0] (inactive) 2012-06-15 08:17:03 PDT
Created attachment 633535 [details] [diff] [review]
now with proper unicode support (m-c)

I pushed this to try and didn't see any issues.
Comment 22 Siddharth Agarwal [:sid0] (inactive) 2012-06-15 08:17:58 PDT
Created attachment 633536 [details] [diff] [review]
now with proper unicode support (c-c)
Comment 23 Siddharth Agarwal [:sid0] (inactive) 2012-06-22 12:48:09 PDT
Created attachment 635871 [details] [diff] [review]
patch updated to fix bitrot (m-c)
Comment 24 Ted Mielczarek [:ted.mielczarek] 2012-06-29 11:34:30 PDT
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.
Comment 25 Ted Mielczarek [:ted.mielczarek] 2012-06-29 11:36:48 PDT
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.
Comment 26 Siddharth Agarwal [:sid0] (inactive) 2012-06-30 04:11:20 PDT
Created attachment 638097 [details] [diff] [review]
what I'm going to check in (m-c)

> 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.
Comment 27 Siddharth Agarwal [:sid0] (inactive) 2012-06-30 04:12:37 PDT
Created attachment 638098 [details] [diff] [review]
what I'm going to check in (c-c)

The two patches are blocked on the ones in bug 757252. I'll check them in together.

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