585011 - Invoke cl.py as a pymake native command

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

We should invoke cl.py as a pymake native command to avoid the overhead of spawning a new Python interpreter every time we run the compiler. This will probably look something like:

ifdef .PYMAKE
PYCOMMANDPATH = $(topsrcdir)/build
CC_WRAPPER = %cl InvokeClWithDependencyGeneration
endif

Comment 1

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

Attachment: Patch

I really want to time this but I am tied up till at least next week.

Comment 2

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

Btw we should approve this because faster builds are awesome.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 464002 [details] [diff] [review]
Patch

>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>@@ -165,6 +165,17 @@ JEMALLOC_LIBS = $(MKSHLIB_FORCE_ALL) $(c
> endif
> endif
> 
>+ifneq (,$(_MSC_VER))
>+ifeq (,$(.PYMAKE))

nit:
ifdef _MSC_VER
ifdef .PYMAKE

alternately:
ifneq(,$(_MSC_VER)$(.PYMAKE))

Comment 4

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

Attachment: Patch

Grr NSS.

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 464116 [details] [diff] [review]
Patch

>+ifndef CC_WRAPPER
>+CC_WRAPPER = $(PYTHON) -O $(topsrcdir)/build/cl.py
>+endif
>+ifndef CXX_WRAPPER
>+CXX_WRAPPER = $(PYTHON) -O $(topsrcdir)/build/cl.py
>+endif

nit: We should be able to assume if one is defined, both are...

ifneq(,$(CC_WRAPPER)$(CXX_WRAPPER))

will make this block read much much simpler.

Comment 6

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

Attachment: NSS makes me cry

Callek suggests this instead.

Comment 7

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

Comment on attachment 464129 [details] [diff] [review]
NSS makes me cry

No reason to approve this since native commands were backed out.

Comment 8

[Andrew McRae]

Comment on attachment 464129 [details] [diff] [review]
NSS makes me cry

+ifdef .PYMAKE
+CC_WRAPPER = $(PYTHON) -O $(topsrcdir)/build/cl.py
+CXX_WRAPPER = $(PYTHON) -O $(topsrcdir)/build/cl.py
+else
+PYCOMMANDPATH += $(topsrcdir)/build
+CC_WRAPPER = %cl InvokeClWithDependencyGeneration
+CXX_WRAPPER = %cl InvokeClWithDependencyGeneration
+endif # .PYMAKE

Shouldn't this be ifndef .PYMAKE ?

Testing with the native commands pymake branch and this patch as is,
a pymake build was still invoking an external python process for cl.py
and a gmake build died pretty quickly with
make[4]: %cl: Command not found

Comment 9

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

Yes.  You are correct.

Comment 10

[Mitchell Field [:Mitch]]

Attachment: Updated patch

This is a fixed-up version of the patch. Carrying forward r+.

Comment 11

[Benjamin Smedberg]

Assuming this has passed tryserver, a=me, though if it bounces at all let's just wait until after we branch.

Comment 12

[Justin Wood (:Callek)]

Mitch/khuey any chance I can convince you to land this on c-c as well with rs+=me

Comment 13

[Mitchell Field [:Mitch]]

I landed this on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/0e9ba7c029e3

Comment 14

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

We'll tackle this after branch.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 483965 [details] [diff] [review]
Updated patch

(bookkeeping)

Comment 16

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

Handing this off to someone who might do it ;-)

Comment 18

[Rain [:rain1] (inactive)]

Attachment: updated patch

I'm no longer seeing the issue described in bug 615571, so we should get this in.

There's a couple of dep bugs: bug 780508, which affects silent mode, and a dep bug with Pymake that I'm about to file.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 649264 [details] [diff] [review]
updated patch

Review of attachment 649264 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/config.mk
@@ +123,5 @@
> +ifdef _MSC_VER
> +ifdef .PYMAKE
> +PYCOMMANDPATH += $(topsrcdir)/build
> +CC_WRAPPER ?= %cl InvokeClWithDependencyGeneration
> +CXX_WRAPPER ?= %cl InvokeClWithDependencyGeneration

Any particular reason you're using ?= here?

Comment 20

[Rain [:rain1] (inactive)]

(In reply to Ted Mielczarek [:ted] from comment #19)
> Comment on attachment 649264 [details] [diff] [review]
> updated patch
> 
> Review of attachment 649264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/config.mk
> @@ +123,5 @@
> > +ifdef _MSC_VER
> > +ifdef .PYMAKE
> > +PYCOMMANDPATH += $(topsrcdir)/build
> > +CC_WRAPPER ?= %cl InvokeClWithDependencyGeneration
> > +CXX_WRAPPER ?= %cl InvokeClWithDependencyGeneration
> 
> Any particular reason you're using ?= here?

So that https://mxr.mozilla.org/mozilla-central/source/security/manager/Makefile.in#12 works.

Comment 21

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0dee35fee533

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0dee35fee533

Comment 23

[Rain [:rain1] (inactive)]

I just backed this out because of bug 794490.

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

Comment 24

[Ed Morley [:emorley]]

(In reply to Siddharth Agarwal [:sid0] from comment #23)
> I just backed this out because of bug 794490.
> 
> http://hg.mozilla.org/integration/mozilla-inbound/rev/ef2e2d800539

https://hg.mozilla.org/mozilla-central/rev/ef2e2d800539

Comment 26

[Gregory Szorc [:gps]]

Attachment: Move cl.py to mozbuild

This is currently humming away fine on my Windows machine \o/. Previous
issues discussed in this bug should be resolved by cl.py switching to
mozprocess (hopefully).

https://tbpl.mozilla.org/?tree=Try&rev=11c7703a9e13

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 816082 [details] [diff] [review]
Move cl.py to mozbuild

Review of attachment 816082 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/config.mk
@@ +137,5 @@
>  MOZ_WIDGET_SUPPORT_LIBS    = $(DIST)/lib/$(LIB_PREFIX)widgetsupport_s.$(LIB_SUFFIX)
>  
>  ifdef _MSC_VER
> +CC_WRAPPER = $(call py_action,cl)
> +CXX_WRAPPER = $(call py_action,cl)

You're changing the ?= to a straight =. I don't remember why it was that way in the first place, though.

::: python/mozbuild/mozbuild/action/cl.py
@@ +51,5 @@
>          if arg.startswith("-Fo"):
>              target = arg[3:]
>              break
>  
>      if target == None:

Can you change this to "target is None" while you're here?

Comment 28

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2131ecb804a8

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2131ecb804a8

Comment 30

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #27)
> Comment on attachment 816082 [details] [diff] [review]
> Move cl.py to mozbuild
> 
> Review of attachment 816082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/config.mk
> @@ +137,5 @@
> >  MOZ_WIDGET_SUPPORT_LIBS    = $(DIST)/lib/$(LIB_PREFIX)widgetsupport_s.$(LIB_SUFFIX)
> >  
> >  ifdef _MSC_VER
> > +CC_WRAPPER = $(call py_action,cl)
> > +CXX_WRAPPER = $(call py_action,cl)
> 
> You're changing the ?= to a straight =. I don't remember why it was that way
> in the first place, though.

Not sure it was its purpose, but currently, that ?= is what makes this work:
https://mxr.mozilla.org/mozilla-central/source/security/build/Makefile.in#6

The switch to = is what causes bug 929983

Comment 31

[Phil Ringnalda (:philor)]

Rumor has it that the very unhelpful failure logs in https://tbpl.mozilla.org/php/getParsedLog.php?id=29748095&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=29734520&tree=Try and https://tbpl.mozilla.org/php/getParsedLog.php?id=29743863&tree=Try are the result of this.