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)
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)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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
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.
Reporter | ||
Comment 3•14 years ago
|
||
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?
Grr NSS.
Attachment #464116 -
Flags: review?(mitchell.field)
Attachment #464116 -
Flags: approval2.0?
Comment 5•14 years ago
|
||
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+
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?
Updated•14 years ago
|
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 8•14 years ago
|
||
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
Yes. You are correct.
Attachment #464129 -
Flags: review?(mitchell.field)
Comment 10•14 years ago
|
||
This is a fixed-up version of the patch. Carrying forward r+.
Attachment #464129 -
Attachment is obsolete: true
Attachment #483965 -
Flags: review+
Updated•14 years ago
|
Attachment #483965 -
Flags: approval2.0?
Comment 11•14 years ago
|
||
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+
Comment 12•14 years ago
|
||
Mitch/khuey any chance I can convince you to land this on c-c as well with rs+=me
Comment 13•14 years ago
|
||
I landed this on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/0e9ba7c029e3
Reporter | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We'll tackle this after branch.
Whiteboard: [pymake]
Comment 15•14 years ago
|
||
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
Comment 18•12 years ago
|
||
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)
Reporter | ||
Comment 19•12 years ago
|
||
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+
Comment 20•12 years ago
|
||
(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•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/0dee35fee533
Assignee: mitchell.field → sagarwal
Status: REOPENED → ASSIGNED
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0dee35fee533
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 23•12 years ago
|
||
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]
Comment 24•12 years ago
|
||
(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
Assignee | ||
Comment 26•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: sid.bugzilla → gps
Reporter | ||
Comment 27•11 years ago
|
||
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+
Assignee | ||
Comment 28•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2131ecb804a8
Status: REOPENED → ASSIGNED
Flags: in-testsuite-
Comment 30•11 years ago
|
||
(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
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [pymake][leave open] → [pymake]
Comment 31•11 years ago
|
||
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.
Updated•11 years ago
|
Target Milestone: mozilla17 → mozilla27
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•