Closed Bug 680636 Opened 8 years ago Closed 7 years ago

Execute nsinstall.py natively under PyMake

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla16

People

(Reporter: gps, Assigned: rain1)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

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.
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 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.
(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.
I wouldn't bother with the comment at all, honestly. We haven't had any comments like that elsewhere.
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.
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
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.
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
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
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.
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.
Backed out of inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/408b90991d17
Target Milestone: mozilla9 → ---
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.
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.
Depends on: 680968
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)
Attachment #628066 - Flags: review?(ted.mielczarek)
Depends on: 757252
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)
Attachment #628066 - Attachment is obsolete: true
Attachment #628066 - Flags: review?(ted.mielczarek)
Attachment #633536 - Flags: review?(ted.mielczarek)
Attachment #633535 - Attachment is obsolete: true
Attachment #633535 - Flags: review?(ted.mielczarek)
Attachment #635871 - Flags: review?(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.
Attachment #633536 - Flags: review?(ted.mielczarek) → review+
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+
> 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+
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+
http://hg.mozilla.org/mozilla-central/rev/88aaf6c529b9
http://hg.mozilla.org/comm-central/rev/a013d9521c46
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 770558
Depends on: 770702
Depends on: 772186
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.