Closed Bug 585011 Opened 14 years ago Closed 11 years ago

Invoke cl.py as a pymake native command

Categories

(Firefox Build System :: General, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(status2.0 wontfix)

RESOLVED FIXED
mozilla27
Tracking Status
status2.0 --- wontfix

People

(Reporter: ted, Assigned: gps)

References

Details

(Whiteboard: [pymake])

Attachments

(2 files, 4 obsolete files)

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
Attached patch Patch (obsolete) — Splinter Review
I really want to time this but I am tied up till at least next week.
Assignee: nobody → me
Status: NEW → ASSIGNED
Attachment #464002 - Flags: review?
Attachment #464002 - Flags: approval2.0?
Attachment #464002 - Flags: review? → review?(mitchell.field)
Btw we should approve this because faster builds are awesome.
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))
Attachment #464002 - Attachment is obsolete: true
Attachment #464002 - Flags: review?(mitchell.field)
Attachment #464002 - Flags: approval2.0?
Attached patch Patch (obsolete) — Splinter Review
Grr NSS.
Attachment #464116 - Flags: review?(mitchell.field)
Attachment #464116 - Flags: approval2.0?
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.
Attachment #464116 - Flags: review+
Attached patch NSS makes me cry (obsolete) — Splinter Review
Callek suggests this instead.
Attachment #464116 - Attachment is obsolete: true
Attachment #464129 - Flags: review?(mitchell.field)
Attachment #464129 - Flags: feedback?(bugspam.Callek)
Attachment #464129 - Flags: approval2.0?
Attachment #464116 - Flags: review?(mitchell.field)
Attachment #464116 - Flags: approval2.0?
Attachment #464129 - Flags: feedback?(bugspam.Callek) → feedback+
Comment on attachment 464129 [details] [diff] [review]
NSS makes me cry

No reason to approve this since native commands were backed out.
Attachment #464129 - Flags: approval2.0?
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
Attached patch Updated patch (obsolete) — Splinter Review
This is a fixed-up version of the patch. Carrying forward r+.
Attachment #464129 - Attachment is obsolete: true
Attachment #483965 - Flags: review+
Attachment #483965 - Flags: approval2.0?
Assuming this has passed tryserver, a=me, though if it bounces at all let's just wait until after we branch.
Attachment #483965 - Flags: approval2.0? → approval2.0+
Mitch/khuey any chance I can convince you to land this on c-c as well with rs+=me
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 615571
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 483965 [details] [diff] [review]
Updated patch

(bookkeeping)
Attachment #483965 - Flags: approval2.0+ → approval2.0-
Handing this off to someone who might do it ;-)
Assignee: khuey → mitchell.field
Attached patch updated patchSplinter Review
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.
Attachment #483965 - Attachment is obsolete: true
Attachment #649264 - Flags: review?(ted.mielczarek)
Depends on: 780508
Depends on: 780612
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?
Attachment #649264 - Flags: review?(ted.mielczarek) → review+
(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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/0dee35fee533
Assignee: mitchell.field → sagarwal
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/0dee35fee533
Status: ASSIGNED → RESOLVED
Closed: 14 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
I just backed this out because of bug 794490.

http://hg.mozilla.org/integration/mozilla-inbound/rev/ef2e2d800539
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [pymake] → [pymake][leave open]
Depends on: 794490
Depends on: 787536
Blocks: 807066
Depends on: 799189
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
Attachment #816082 - Flags: review?(ted)
Assignee: sid.bugzilla → gps
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?
Attachment #816082 - Flags: review?(ted) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2131ecb804a8
Status: REOPENED → ASSIGNED
Flags: in-testsuite-
(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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [pymake][leave open] → [pymake]
Target Milestone: mozilla17 → mozilla27
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.