680636 - Execute nsinstall.py natively under PyMake

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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]]

Attachment: 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]]

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]]

(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

[(not currently active) Ted Mielczarek]

I wouldn't bother with the comment at all, honestly. We haven't had any comments like that elsewhere.

Comment 5

[(not currently active) Ted Mielczarek]

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 (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

Also, in the future, 8 lines of context would be better.

Comment 8

[Gregory Szorc [:gps]]

Attachment: 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]]

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 (Exited; not receiving bugmail, old account, do not use)]

Try doesn't run pymake so you're not going to learn that much.

Comment 11

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e10579e136f1

Comment 12

[Mozilla RelEng Bot]

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/[e-mail]-6f63ee0f78d4

Comment 13

[Ed Morley [:emorley]]

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]]

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]]

Backed out of inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/408b90991d17

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ugh, that's pretty nasty :-/

Comment 17

[Gregory Szorc [:gps]]

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

[(not currently active) Ted Mielczarek]

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

[Rain [:rain1] (inactive)]

Attachment: handle a large subset of nsinstall invocations (m-c)

This patch goes on top of the one in bug 757252.

Comment 20

[Rain [:rain1] (inactive)]

Attachment: handle a large subset of nsinstall invocations (c-c)

Comment 21

[Rain [:rain1] (inactive)]

Attachment: now with proper unicode support (m-c)

I pushed this to try and didn't see any issues.

Comment 22

[Rain [:rain1] (inactive)]

Attachment: now with proper unicode support (c-c)

Comment 23

[Rain [:rain1] (inactive)]

Attachment: patch updated to fix bitrot (m-c)

Comment 24

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

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

[Rain [:rain1] (inactive)]

Attachment: 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

[Rain [:rain1] (inactive)]

Attachment: 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.

Comment 28

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/88aaf6c529b9
http://hg.mozilla.org/comm-central/rev/a013d9521c46