Closed
Bug 461444
Opened 16 years ago
Closed 15 years ago
remove cases of excessive recursion in makefiles
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: Mitch)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 11 obsolete files)
19.38 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
2.00 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
While working on bug 461395, going through the makefiles in content/, I noticed some ridiculous directory structures. The worst of the worst was content/mathml, which goes all the way down to: http://mxr.mozilla.org/mozilla-central/source/content/mathml/content/src/ Neither content/mathml nor content/mathml/content have any files in them except for a makefile with a DIRS line. This means that just to build those few source files we have to invoke make three times. Every process we spawn makes the build a bit slower, especially on Windows. There's no reason that source couldn't be in content/mathml. If we have directory structures that we really do want to preserve, we can just do DIRS = foo/bar instead of having a directory with nothing but a makefile and DIRS. There are a few other places just in content/ where we could improve things. I heard there was a session at the summit about organization of source directories, and supposedly roc took notes. I don't particularly care if we flatten the public/ src/ dirs, as they might be nice for file organization, but that directory structure means that we invoke 'make export' in both, and 'make libs' in both, meaning we wind up with two no-op makes every time we build that directory. If we don't want to flatten, we could just use "VPATH = public src" in the directory containing them, and move all the functional bits out of {public,src}/Makefile.in into the upper level makefile. vlad mentioned on IRC that he was opposed to pure flattening of the directories. Regardless, if you're interested, I can draw up a proposal in the form of a patch, and do some measurements on build speed.
I took notes and posted them to dev.platform, IIRC. I thought we agreed to combine public and src across the board. So all the MathML content files would end up in content/mathml.
Comment 2•16 years ago
|
||
Could we get some of these suggested improvements up on a wiki page or newsgroup post or something? I'm thinking we want to adopt some of them for comm-central, and it'll be much easy if we have a wiki page of how to optimise the build system (and for future reference), rather than digging around in bugs.
Reporter | ||
Comment 3•16 years ago
|
||
(In reply to comment #1) > I took notes and posted them to dev.platform, IIRC. > > I thought we agreed to combine public and src across the board. So all the > MathML content files would end up in content/mathml. I can't find these notes anywhere. Do you still have a local copy?
Comment 4•16 years ago
|
||
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/27549f4e2fb4f51a/f058d235a0f68c60?lnk=gst&q=robert+public+src#f058d235a0f68c60
Assignee | ||
Comment 5•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #406838 -
Flags: review?(ted.mielczarek)
Comment 6•15 years ago
|
||
Comment on attachment 406838 [details] [diff] [review] toolkit I'm happy for ted to review this, but I think the whitespace changes make it hard to see what the real changes are... there appear to be at least a few real changes adding qt in some platform checks.
Attachment #406838 -
Flags: review?(benjamin)
Assignee | ||
Comment 7•15 years ago
|
||
The previous patch bitrotted quickly. Please review this one as soon as possible.
Attachment #406838 -
Attachment is obsolete: true
Attachment #408173 -
Flags: review?(ted.mielczarek)
Attachment #406838 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 8•15 years ago
|
||
Comment on attachment 408173 [details] [diff] [review] toolkit (v2) -DEPTH = .. +DEPTH = .. topsrcdir = @top_srcdir@ -srcdir = @srcdir@ -VPATH = @srcdir@ +srcdir = @srcdir@ +VPATH = @srcdir@ These intentionally have the = lined up. Why are you changing that? There aren't any hard tabs in there, so it's fine, just leave it. The hard tab removal elsewhere is nice, though. --- a/toolkit/components/alerts/Makefile.in +EXTRA_DSO_LDOPTS += $(MOZ_COMPONENT_LIBS) This isn't actually necessary here, since we're just making a static library. Just remove it. --- a/toolkit/components/autocomplete/Makefile.in +EXTRA_DSO_LDOPTS += \ + $(MOZ_UNICHARUTIL_LIBS) \ + $(MOZ_COMPONENT_LIBS) \ + $(NULL) + You left some hard tabs in there, switch them out. --- a/toolkit/components/feeds/Makefile.in ABS_SRCDIR := $(shell cd $(srcdir) && pwd) ifeq ($(OS_ARCH),WINNT) ABS_DEPTH := $(shell cd $(DEPTH) && pwd) While you're touching every makefile, care to change these to: $(call core_abspath,$(whatever)) ? --- a/toolkit/components/filepicker/Makefile.in +nsFilePicker.js: nsFilePicker.js.in + $(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) $^ > $@ endif endif include $(topsrcdir)/config/rules.mk This nsFilePicker.js rule has to come after rules.mk, otherwise it becomes the default rule for the makefile. --- a/toolkit/components/passwordmgr/Makefile.in -DIRS = public src content +DIRS = content The Makefile in toolkit/components/passwordmgr/content is totally useless. Just remove content from DIRS here, and rm that Makefile.in. (The files in there are processed by the jar.mn in this directory.) --- a/toolkit/components/satchel/Makefile.in +EXTRA_DSO_LDOPTS += \ + $(LIBS_DIR) \ + $(EXTRA_DSO_LIBS) \ + $(MOZ_UNICHARUTIL_LIBS) \ + $(MOZ_COMPONENT_LIBS) \ + $(NULL) + +ifdef MOZ_MORKREADER +EXTRA_DSO_LDOPTS += $(DEPTH)/db/morkreader/$(LIB_PREFIX)morkreader_s.$(LIB_SUFFIX) +endif These EXTRA_DSO_LDOPTS need to go after rules.mk. (In general, EXTRA_DSO_LDOPTS should go after rules.mk, but it only really matters if you also have EXTRA_DSO_LIBS, I think.) --- a/toolkit/mozapps/update/src/updater/Makefile.in libs:: updater.png - $(NSINSTALL) -D $(DIST)/bin/icons - $(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/icons + $(NSINSTALL) -D $(DIST)/bin/icons + $(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/icons libs:: - $(NSINSTALL) -D $(DIST)/bin/updater.app - rsync -a -C --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/bin/updater.app - sed -e "s/%APP_NAME%/$(MOZ_APP_DISPLAYNAME)/" $(srcdir)/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \ - iconv -f UTF-8 -t UTF-16 > $(DIST)/bin/updater.app/Contents/Resources/English.lproj/InfoPlist.strings - $(NSINSTALL) -D $(DIST)/bin/updater.app/Contents/MacOS - $(NSINSTALL) $(DIST)/bin/updater $(DIST)/bin/updater.app/Contents/MacOS - rm -f $(DIST)/bin/updater + $(NSINSTALL) -D $(DIST)/bin/updater.app + rsync -a -C --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/bin/updater.app + sed -e "s/%APP_NAME%/$(MOZ_APP_DISPLAYNAME)/" $(srcdir)/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \ + iconv -f UTF-8 -t UTF-16 > $(DIST)/bin/updater.app/Contents/Resources/English.lproj/InfoPlist.strings + $(NSINSTALL) -D $(DIST)/bin/updater.app/Contents/MacOS + $(NSINSTALL) $(DIST)/bin/updater $(DIST)/bin/updater.app/Contents/MacOS + rm -f $(DIST)/bin/updater Looks like you got a little carried away with tab replacement. Those are rule commands, they need to stay as literal tabs. Looks pretty good otherwise. When you fix those issues, can you create a bundle of your changeset and attach that as well? I can push it to the try server for you then, so we can make sure we didn't miss anything. Thanks!
Attachment #408173 -
Flags: review?(ted.mielczarek) → review-
Assignee | ||
Comment 9•15 years ago
|
||
Attachment #408173 -
Attachment is obsolete: true
Attachment #409286 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•15 years ago
|
Attachment #409286 -
Attachment is obsolete: true
Attachment #409286 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 10•15 years ago
|
||
This patch removes 40 makefiles from toolkit.
Attachment #412549 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 11•15 years ago
|
||
Tests should be fixed now.
Attachment #412549 -
Attachment is obsolete: true
Attachment #413012 -
Flags: review?(ted.mielczarek)
Attachment #412549 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 12•15 years ago
|
||
Updated.
Attachment #413012 -
Attachment is obsolete: true
Attachment #413604 -
Flags: review?(ted.mielczarek)
Attachment #413012 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 13•15 years ago
|
||
Reporter | ||
Comment 14•15 years ago
|
||
Comment on attachment 413604 [details] [diff] [review] toolkit (v6) Ok, this looks good. I'm giving it a run past the try server and if everything looks ok I'll push it. Sorry for the delay, thanks for your patience! These are great patches, they're just tough to review. I really appreciate the work you're doing here.
Attachment #413604 -
Flags: review?(ted.mielczarek) → review+
Reporter | ||
Comment 15•15 years ago
|
||
There was a little build error on Try WinMo, I fixed it locally. More troubling, mochitest-chrome timed out on all three platforms, but in a different place on OS X than Linux/Windows: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1261148004.1261157822.31696.gz#err10 http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1261148004.1261156623.17042.gz#err12 http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1261148004.1261159712.20454.gz#err11 Need to figure out what's happening there before landing this.
Assignee | ||
Comment 16•15 years ago
|
||
I'm breaking the massive patch down into more manageable pieces. Please review this patch and push to TryServer and mozilla-central if all goes well.
Attachment #413604 -
Attachment is obsolete: true
Attachment #413608 -
Attachment is obsolete: true
Attachment #419572 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 17•15 years ago
|
||
Sorry, caught a mistake in that last one.
Attachment #419572 -
Attachment is obsolete: true
Attachment #419574 -
Flags: review?(ted.mielczarek)
Attachment #419572 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 18•15 years ago
|
||
Attachment #419574 -
Attachment is obsolete: true
Attachment #421439 -
Flags: review?(ted.mielczarek)
Attachment #419574 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 19•15 years ago
|
||
Attachment #421439 -
Attachment is obsolete: true
Attachment #421467 -
Flags: review?(ted.mielczarek)
Attachment #421439 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #421467 -
Attachment is obsolete: true
Attachment #421645 -
Flags: review?(ted.mielczarek)
Attachment #421467 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 21•15 years ago
|
||
Comment on attachment 421645 [details] [diff] [review] toolkit/mozapps (v5) This looks sensible. I've run it past the try server before and it looked ok, so I'll push it to m-c shortly.
Attachment #421645 -
Flags: review?(ted.mielczarek) → review+
Reporter | ||
Comment 22•15 years ago
|
||
Pushed this to m-c: http://hg.mozilla.org/mozilla-central/rev/83eac7a5262a Mitch: do you want to move to a new bug for followups?
Comment 23•15 years ago
|
||
(In reply to comment #8) > --- a/toolkit/components/filepicker/Makefile.in > +nsFilePicker.js: nsFilePicker.js.in > + $(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) $^ We ought to rename this to nsFilePicker.js and add it to EXTRA_PP_COMPONENTS
Comment 24•15 years ago
|
||
If this is done to mozapps/update please rename nsUpdateService.js.in to nsUpdateService.js at the same time. Thanks!
Assignee | ||
Comment 25•15 years ago
|
||
Thanks, Neil. I'll handle toolkit/components in another bug report, and I've now made use of EXTRA_PP_COMPONENTS in the files already touched by the previous patch.
Attachment #423512 -
Flags: review?(ted.mielczarek)
Reporter | ||
Updated•15 years ago
|
Attachment #423512 -
Flags: review?(ted.mielczarek) → review+
Reporter | ||
Comment 26•15 years ago
|
||
Pushed the followup to m-c: http://hg.mozilla.org/mozilla-central/rev/0b2369a6ae91 Followup work can be done in a new bug.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 27•15 years ago
|
||
Relanded both those patches as http://hg.mozilla.org/mozilla-central/rev/3d6a406c0067 to fix the file renames
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
•