562313 - allow linking browser components directly into libxul

Status: NEW

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

We currently ship components/libbrowsercomps.so with Firefox's binary components. We should be able to (with a little bit of build trickery) link those components directly into libxul instead. It should continue to get built as a standalone library when building --with-libxul-sdk, however.

Comment 1

[(not currently active) Ted Mielczarek]

Here's my plan:
1) add a variable that you can set in build.mk before including toolkit-tiers.mk that adds a dir to the platform tier right before building toolkit/library, use this variable in browser/build.mk to build browser/components before linking libxul
2) add a variable somewhere that makes nsStaticXULComponents include a header or something to add components to that list, include the guts of browser/components/build/nsModule.cpp in there
3) add a variable that tells libxul to link in some extra libs, link in libbrowsercomps (which will have been switched to a static lib)

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: Part 1: add hooks to libxul to allow linking in app components

This patch allows apps to:
1) Set APP_LIBXUL_DIRS before including toolkit-tiers.mk, resulting in those directories being built right before toolkit/library.
2) Set MOZ_APP_COMPONENT_INCLUDE to a header filename in confvars.sh. The header file will be included in nsStaticXULComponents.cpp and should #define APP_COMPONENT_MODULES appropriately (like MODULE(nsBrowserCompsModule))
3) Set MOZ_APP_COMPONENT_LIBS to the name of the static library containing the app components (also in confvars.sh).

Thus, the modules can get built and linked into libxul.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: Part 2: Link browsercomps into libxul using the hooks

This patch uses the hooks described above to link browsercomps into libxul. Most of this patch is fiddling to make external vs. internal string apis work right.

This built for me locally on Linux, I pushed it to try for a sanity check. (It's quite possible some of the migration or shell code needs more platform-specific string api fixes.)

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 442800 [details] [diff] [review]
Part 2: Link browsercomps into libxul using the hooks

This patch needs work, fails on Windows and Mac.

Comment 5

[(not currently active) Ted Mielczarek]

Hrmph. Worked on my local Mac build, might just be Universal build bustage.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: Part 2 (v2): Link browsercomps into libxul using the hooks

Fixed the Windows bustage.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: Part 2 (v2): Link browsercomps into libxul using the hooks

Oops, wrong patch.

Comment 8

[(not currently active) Ted Mielczarek]

Oops, I forgot about the second patch here. I'll test a universal build locally and finish it up.

Comment 9

[(not currently active) Ted Mielczarek]

Ah, found the Mac bustage. On Mac we symlink all the .o files from the static libs that we use to build libxul into a single directory and then link them into libuxl. Unfortunately with this patch we wound up linking two files named nsModule.o, so we were overwriting one of them and failing to link. I've renamed browser/components/build/nsModule.cpp to nsBrowserCompsModule.cpp and it looks good. I'll have an updated patch shortly.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: Part 2 (v3): Link browsercomps into libxul using the hooks

Comment 11

[(not currently active) Ted Mielczarek]

Accidentally hit return, but that patch builds and works on Linux/Windows/OS X. I haven't tested it in the FF+XR build config, but it's ifdefed such that it should continue to work there.

Comment 12

[Benjamin Smedberg]

Hrm. I don't mean to be stop-energy now, but we could probably do this a lot simpler now by linking these into firefox.exe and using XRE_AddStaticComponent. That wouldn't involve switching on internal/external linkage, or switching the tiers around in this way.

I'm going to try and work up that patch quickly and see if you find it acceptable.

Comment 13

[Benjamin Smedberg]

Attachment: Link browsercomps into firefox.exe statically, rev. 1

Running this through tryserver now, but it basically statically links browsercomps when we're building standalone, and builds a binary component when we're building --with-libxul-sdk (Linux distro case)

Comment 14

[Michael Wu [:mwu]]

(In reply to comment #13)
> Created attachment 462897 [details] [diff] [review]
> Link browsercomps into firefox.exe statically, rev. 1
> 
> Running this through tryserver now, but it basically statically links
> browsercomps when we're building standalone, and builds a binary component when
> we're building --with-libxul-sdk (Linux distro case)

Hm, why not libxul? The problem with linking into the program is that we don't use the program on Android.. we just load libxul.

Comment 15

[Benjamin Smedberg]

What does 'use the program' mean? Presumably you have *a* program, no?

Comment 16

[Michael Wu [:mwu]]

(In reply to comment #15)
> What does 'use the program' mean? Presumably you have *a* program, no?
We have a fennec binary. We don't run it or package it. libxul is loaded directly from the JVM and a special loading function in nsAndroidStartup is used to set things up and call XRE_main.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 462897 [details] [diff] [review]
Link browsercomps into firefox.exe statically, rev. 1

># HG changeset patch
># Parent a0694218d21e81f84dc8278eaa78c0c869f98990
>Bug 562313 - Statically link binary browser components into firefox.exe, r?ted
>
>diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in
>--- a/browser/app/Makefile.in
>+++ b/browser/app/Makefile.in
>@@ -89,16 +89,22 @@ else
> ifneq (,$(filter OS2 WINCE WINNT,$(OS_ARCH)))
> PROGRAM = $(MOZ_APP_NAME)$(BIN_SUFFIX)
> else
> PROGRAM = $(MOZ_APP_NAME)-bin$(BIN_SUFFIX)
> endif
> 
> CPPSRCS = nsBrowserApp.cpp
> 
>+LIBS += \
>+	$(call EXPAND_LIBNAME_PATH,browsercomps,../components/build) \
>+	$(call EXPAND_LIBNAME_PATH,unicharutil_external_s,$(LIBXUL_DIST)/lib) \
>+	$(LIBXUL_DIST)/lib/$(LIB_PREFIX)mozreg_s.$(LIB_SUFFIX) \
>+	$(NULL)

Nit: 2-space indent, not tabs.

Much nicer than my patches, certainly.

Comment 18

[(not currently active) Ted Mielczarek]

Oh, you will need to patch package-manifest.in and removed-files.in as well. You can just snag the hunks out of attachment 460608 [details] [diff] [review].

Comment 19

[(not currently active) Ted Mielczarek]

Do you have time to update this patch, or do you want me to update it and request approval? It'd be nice to knock out one more shared library for FF4 since we have this patch in hand.

Comment 20

[Benjamin Smedberg]

Hrm, I forgot about it. Pre-approved, or I can maybe do it later today.

Comment 21

[Benjamin Smedberg]

Attachment: Link browsercomps into firefox.exe statically, rev. 2

Here's what I'm rebuilding for push today.

Comment 22

[Benjamin Smedberg]

Attachment: Link browsercomps into firefox.exe statically, rev. 2.1

Whoops, got an ifdef reversed. This works.

Comment 23

[Bas Schouten (:bas.schouten)]

Pushed http://hg.mozilla.org/mozilla-central/rev/7c0438df6767

Bustage in:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282947715.1282948921.1154.gz&fulltext=1#err0
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282947771.1282948993.1363.gz&fulltext=1

Backed out http://hg.mozilla.org/mozilla-central/rev/4bbe896fbc03

Comment 24

[Daniel Holbert [:dholbert]]

Looks like this might need a clobber in order to build successfully the first time.

By chance, it got one cycle that was a clobber:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282948127.1282952807.18472.gz
and that went green.

If bsmedberg can confirm that this makes sense (and that this has been green on tryserver at some point), then this probably needs a re-push.

Comment 25

[Daniel Holbert [:dholbert]]

The green build cycle had an orange xpcshell cycle, though:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282951713.1282952974.18985.gz
with something failing in build/xpcshell/tests/browser/components/dirprovider/tests/unit/test_bookmark_pref.js

That's not a known randomorange, and the next cycle (backout) was green for xpcshell, so it seems plausible that this orange was related to this bug.

Comment 26

[Daniel Holbert [:dholbert]]

Here's another instance of that xpcshell orange (from another build with this changeset that was lucky enough to be a clobber & build successfully):
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282951248.1282952849.18558.gz

Comment 27

[Michael Wu [:mwu]]

(In reply to comment #25)
> The green build cycle had an orange xpcshell cycle, though:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282951713.1282952974.18985.gz
> with something failing in
> build/xpcshell/tests/browser/components/dirprovider/tests/unit/test_bookmark_pref.js
> 
> That's not a known randomorange, and the next cycle (backout) was green for
> xpcshell, so it seems plausible that this orange was related to this bug.

It appears that this test relies on browsercomps being loaded, but it can't be now that it's linked into the firefox binary. Looks like this patch breaks any xpcshell tests of browsercomps.

Comment 28

[(not currently active) Ted Mielczarek]

Standard8 landed my original approach in bug 597465, so we might as well just land my browser patch from here for now, and then fix things to be less crappy post-Firefox 4.

Comment 29

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

Comment on attachment 469916 [details] [diff] [review]
Link browsercomps into firefox.exe statically, rev. 2.1

This was approved back in 2010, and never landed due to difficulties. Removing the approval for now. Feel free to renominate with risk v. reward if you want to see it in the tree for RC.

Comment 30

[Mike Hommey [:glandium]]

Binary components have all been merged into libxul a long time ago.