(not currently active) Ted Mielczarek Reporter
	Description • 16 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 1 • 16 years ago

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.

	Mark Banner (:standard8)
	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.

	(not currently active) Ted Mielczarek 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?

	Benjamin Smedberg
	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

	Benjamin Smedberg
	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.

	Mitchell Field [:Mitch] Assignee
	Comment 7 • 15 years ago

The previous patch bitrotted quickly. Please review this one as soon as possible.

	(not currently active) 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!

	Mitchell Field [:Mitch] Assignee
	Comment 10 • 15 years ago

This patch removes 40 makefiles from toolkit.

	Mitchell Field [:Mitch] Assignee
	Comment 11 • 15 years ago

Tests should be fixed now.

	Mitchell Field [:Mitch] Assignee
	Comment 12 • 15 years ago

Updated.

	(not currently active) Ted Mielczarek 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.

	(not currently active) Ted Mielczarek 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.

	Mitchell Field [:Mitch] 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.

	Mitchell Field [:Mitch] Assignee
	Comment 17 • 15 years ago

Sorry, caught a mistake in that last one.

	(not currently active) 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.

	(not currently active) Ted Mielczarek 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?

	neil@parkwaycc.co.uk
	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

	Robert Strong (they/them - no direct email)
	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!

	Mitchell Field [:Mitch] 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.

	(not currently active) Ted Mielczarek 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.

	Dave Townsend [:mossop]
	Comment 27 • 15 years ago

Relanded both those patches as http://hg.mozilla.org/mozilla-central/rev/3d6a406c0067 to fix the file renames