469873 - Fennec building does not link XPT files in a single bundle : no call to xptlink.pl

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Frederic Plourde (:fred23)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111317 Ubuntu/8.04 (hardy) Firefox/3.0.4
Build Identifier:  fennec-1.0a2pre.en-US

Fennec (mobile) builds xulrunner and runs on top of it. We noticed that the packager.mk does NOT link the XPT files together as it should be (by calling xptlink.pl), whereas it does it ok for firefox building.

The reason is that the call to xptlink.pl, triggered by MOZ_PKG_MANIFEST define is only made inside browser/installer/Makefile.in, which is used by firefox building, of course, but not fennec. We should relocate that part of the Packaging system at a higher level, like, directly in packager.mk.

Reproducible: Always

Steps to Reproduce:
1. build fennec with a proper mozconfig
2. type "make package" from the /objdir/mobile directory
3. notice that the /objdir/mobile/dist/fennec/xulrunner/component/ directory is full of several XPT files.
Actual Results:  
Actually, no XPT bundle i0s created because xptlink.pl is not called at all for fennec building

Expected Results:  
we expect a single XPT bundle created.

Comment 1

[(not currently active) Ted Mielczarek]

I asked Fred to file this bug. bsmedberg: is there any reason we can't just unconditionally run xptlink in packager.mk, instead of requiring the app to have a packaging manifest? (Which we don't currently have for xulrunner or fennec.)

Comment 2

[Benjamin Smedberg]

I think that's fine.

Comment 3

[Phil Ringnalda (:philor)]

We've been assuming (in bug 420391, where I was using the lack of this as my excuse for giving Tb/Linux a manifest) that this switch then requires removing the individual xpts, so you don't wind up with a modern version of the foo interfaces in app.xpt and an ancient and incompatible one in your still-around foo.xpt. Are we wrong, or does it just not matter than some of the things that would be affected by this don't have a removed-files, since they also don't have a way to update other than the always-discouraged (and, always-done-anyway) untar over an old install?

Comment 4

[Frederic Plourde (:fred23)]

I've linked all XPT files for fennec packaging, and I've got some numbers.
According to the little tests I've run, We gain roughly 0.25 seconds on startup time by loading only 1 XPT bundle. (instead of 131 individual xpts).

Here are the detailled results.


*****************
COMPARATIVE STUDY
*****************
: on the effect of loading 1 big XPT file
  instead of 131 small xpts, at startup.
*****************************************
CONFIGURATION
----------------------
plateform : N810
build config : --disable-debug, --enable_optimize

METHODOLOGY
----------------------
Time readings obtained with a stopwatch
start : commandline "./fennec" input.
stop  : displaying of "fennec" in the title bar.

number of readings : 12 for each case.
1 min and 1 max values were rejected for each case.
remaining startup time readings : 10 for each case.

before each case, compreg.dat and xpti.dat were deleted.
Initial fennec runs were discarded. (building component manifests.)

Raw results :
** WITH 131 XPT FILES**
9.34 sec
9.18 sec
9.22 sec
9.34 sec
9.29 sec
9.34 sec
9.31 sec
9.35 sec
9.26 sec
9.30 sec
mean startup time :  9.293 seconds
	
** WITH 1 XPT BUNDLE FILE **
9.02 sec
8.97 sec
8.96 sec
9.04 sec
9.12 sec
9.05 sec
9.04 sec
9.00 sec
9.14 sec
9.05 sec
mean startup time :  9.04 seconds


DISCUSSION
----------------------

According to this vary quick study, 
We gain aprox. 0.25 seconds on fennec startup time
with XPT linking.

Comment 5

[Frederic Plourde (:fred23)]

Attachment: Patch for explicitely calling xptlink from fennec installer makefile

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 353878 [details] [diff] [review]
Patch for explicitely calling xptlink from fennec installer makefile

landing this on the fennec side for fennec-a2

we can backout and fix on the xulrunner side later

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

hmm, this patch is causing errors during the | make package | step

Comment 8

[Frederic Plourde (:fred23)]

On which plateform did you run it ?

Comment 9

[Frederic Plourde (:fred23)]

mfinkle: 
Could you paste info about those errors, as I reapplied it on my system and nothing went wrong... I guess a quick copy-paste of the make-package-error-messages would be enough for me to fix the issue.

thanx.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> mfinkle: 
> Could you paste info about those errors, as I reapplied it on my system and
> nothing went wrong... I guess a quick copy-paste of the
> make-package-error-messages would be enough for me to fix the issue.

I had to change MOZ_PKG_DIR to MOZ_APP_NAME. But as we missed Fennec A2, I think we should just make a patch for XULRunner.

Comment 11

[Frederic Plourde (:fred23)]

k, I'll work on that XULRunner patch as soon as I come back from vacation, january 5th.

Comment 12

[Frederic Plourde (:fred23)]

Just to shake things up here with another issue that was brought up by philor:

 It seems that linking all XPT files into a single bundle could cause serious conflicts for users installing the product on top of an old one. In fact, if a new "1-XPT-bundle" fennec is installed (make install) on top of an old "many-separate-xpt-files" version of fennec, serious conflicts could arise between old and new xpcom interfaces because it's going to load all separate XPT files along with the XPT bundle, creating redundancy in the loaded components.

patching for this type of problem could be done at the installer level... but certainly not at the app level, as we should not assume the app's going to have write permissions.

question: XPT linkng is already done for FF, so what's being done there to avoid the aforementioned issue ?

Comment 13

[(not currently active) Ted Mielczarek]

On Windows, we do not ship zip builds of releases, and the installer takes care of removing old files. (We have a removed-files.in that both the installer and the updater use to remove old files.)

On Linux, it's sort of a crapshoot. If a user untars a new build on top of an old build, they can certainly break themselves, but that's true regardless of what you do. Does the deb installer not take care of removing the old files first before unpacking new files? If not, that seems pretty terrible in general. What if you have a JS component that you later remove?

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

The worse case, IMO, is to use a special build flag to force XPT linking. Currently, we only link XPTs if a packaging manifest has been defined. Let's change to use a flag or package manifest.

The flag could be set by apps that want the behavior.

Comment 15

[Frederic Plourde (:fred23)]

Attachment: patch for xptlinking on xulrunner side

Mark Finkle and I talked about the best possible locations for xpt-linking stuff. For we wanted to add individual control over the process by adding mozconfig flags in a per-app manner, we aggreed that xpt-linking should be triggered at BUILD TIME (for apps for which --enable-xpt-linking flag was defined). Therefore, the associated patch hacks its way in through /xulrunner/app/Makefile.in

Be aware that a new mozconfig flag has been created with this patch.
--enable-xpt-linking  ou --disable-xpt-linking  to enable or disable the feature. And the default is "--enable"

Notice also that firefox's former way of linking xpts has been kept exactly the same in Packager.mk (except for the added condition 'PKG_SKIP_XPT_LINKING') as I didn't want to break anything already working. If the present patch proves itself useful, we shall relocate package-time-xpt-linking code from Packager.mk to browser/app/Makefile.in. Likewise, any app that wants to enable xptlinking should add the xpt_link call in its /app/Makefile.in, as you see it done in the patch.

Comment 16

[Frederic Plourde (:fred23)]

Attachment: patch making mobile delete previous XPT files before copying over xulrunner dist

The last patch suffers from an inevitable problem : when you build mobile with --enable-xpt-linking over a pre-existing build with --disable-xpt-linking, all the separate XPT files from the previous build are going to be copied over to mobile/dist/bin/xulrunner/components dir, along with the bundled xpt file (xulrunner.xpt).

This very simple 'mobile' patch resolves this problem by removing every xpt files before copying over the xulrunner dist dir.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 357226 [details] [diff] [review]
patch making mobile delete previous XPT files before copying over xulrunner dist

Unless I'm mistaken, this would only be a concern if we did not clobber the DIST folder.

I don't think we need to add this for normal circumstances.

Let me know if I am missing something.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 357225 [details] [diff] [review]
patch for xptlinking on xulrunner side

Better to have Ted look at this

Comment 19

[Benjamin Smedberg]

I may have missed some history here, but Stuart asked if I could review this: I don't understand why we're doing this during the libs phase. We want separate XPT files during the libs phase. Linking them during libs means that either you are removing the non-linked XPTs near the end of the build, or that you're leaving them and have duplicate information. Either way it sucks for developers.

I think you want to link the XPTs during the packaging phase, so that it will take effect when you call `make install` or `make package`.

I also don't understand why this needs to be configurable, or why there needs to be an ifdef in packager.mk

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #19)

> I think you want to link the XPTs during the packaging phase, so that it will
> take effect when you call `make install` or `make package`.

Thinking of the XULRunner app the builds XULRunner, like Fennec, we don't actually call | make install | or | make package | from the XULRunner objdir. Are you referring to the XUL app's package/install phase?

> 
> I also don't understand why this needs to be configurable, or why there needs
> to be an ifdef in packager.mk

There was some discussion that enabling XPT bundling by default might cause some problems for XUL apps that have already installed individual XPTs. The configuration flag was just a way for any potential problems to be avoided.

Comment 21

[Benjamin Smedberg]

As far as I can tell, the default value is to bundle XPTs, so you're not really avoiding the problem.

Comment 22

[Benjamin Smedberg]

And, how are you getting xulrunner into the app directory? If you're just copying xulrunner/dist/bin wholesale, I think you should probably use make package instead and unpackage it.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #21)
> As far as I can tell, the default value is to bundle XPTs, so you're not really
> avoiding the problem.

Well, it gives those apps a way to avoid the problem.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #22)
> And, how are you getting xulrunner into the app directory? If you're just
> copying xulrunner/dist/bin wholesale,

Yes, we are copying wholesale.

> I think you should probably use make
> package instead and unpackage it.

I see. Kinda like the tar/untar way of copying things around. I guess we could do that. I assume | untar | or similar is the way to unpackage a package.

Comment 25

[(not currently active) Ted Mielczarek]

$(UNMAKE_PACKAGE) is the way to go (it's in packager.mk), something like this:
http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#273

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 357225 [details] [diff] [review]
patch for xptlinking on xulrunner side

r- per discussion. Awaiting new patch!

Comment 27

[Frederic Plourde (:fred23)]

Attachment: xptlinking patch on the 'mobile' side

See next comment with its associated patch.

Comment 28

[Frederic Plourde (:fred23)]

Attachment: xptlinking patch on the 'XULrunner' side

After recent discussions with Ben and Ted, here's another patch accomplishing mobile XPTlinking through the following steps:

1) 'mobile' copies over its /dist/bin folder as it used to do, but excludes the /dist/bin/xulrunner sub-dir.
2) 'mobile' explicitely launches xulrunner packaging operation
3) 'xulrunner' packages itself and triggers XPT linking on the way.
4) 'mobile' unmakes xulrunner package in the proper directory using 'UNMAKE_PACKAGE'.

there are two patches... one on the mozilla-central side (Xulrunner_XPTLinking.patch) and one on the 'mobile' side (Mobile_XPTLinking.patch).

* Here are some precisions about the Xulrunner patch:

Xulrunner_XPTLinking.patch
--------------------------
 - To exclude 'xulrunner' dist dir from the mobile copying operation, I've defined and used the new 'TAR_EXCLUDE_LIST' environment variable. Since packager.mk is commonly used by different apps, I didn't want to hard-code any 'mobile-related' stuff inside. So mobile/installer/Makefile.in is take care of defining TAR_EXCLUDE_LIST to 'xulrunner' before including packager.mk.

 - in packager.mk, XPTLinking is enclosed inside a 'ifdef LINK_XPT_WITHOUT_MANIFEST'. This is important, since fennec building doesn't have a package manifest (yet ?).

 - I've enhanced 'xptlink.pl' quite a lot. Initially, I found many incoherences inside the code (like input command-line arguments that are never used!). I tried to reach the initial developers (long gone from mozilla), but in vain. So I hacked the script by adding 2 input arguments ('finalName and oneDir') to allow for xpt-linking of specific directories.


CONCERNS:

 I have two concerns that reviewers might check closer :

1) I used the MOZ_PKG_APPNAME macro in packager.mk for the final XPT bundle filename. Even if it works for me (MOZ_PKG_APPNAME = 'xulrunner'), I remember that Mark Finkle's builds had that macro undefined... so when is it defined ?  when is it not ?

2) In /mobile/installer/Makefile.in (Mobile_XPTlinking.patch), I don't have access to any 'xulrunner-specific-macro', so I hardcoded "xulrunner*$(PKG_SUFFIX)" to feed the UNMAKE_PACKAGE command. I could have written "*$(PKG_SUFFIX) also, but it would unmake any tar.bz2 found in LIBXUL_DIST dir.

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 359322 [details] [diff] [review]
xptlinking patch on the 'XULrunner' side

+	@cd $(DIST)/bin && tar $(TAR_EXCLUDE_LIST) $(TAR_CREATE_FLAGS) - * | (cd ../$(MOZ_PKG_DIR); tar -xf -)

This TAR_EXCLUDE_LIST shouldn't be necessary. This is why we have NO_PKG_FILES:
http://mxr.mozilla.org/mozilla-central/source/xulrunner/installer/Makefile.in#47
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#411

+ifdef LINK_XPT_WITHOUT_MANIFEST
+	$(PERL) $(MOZILLA_DIR)/xpinstall/packager/xptlink.pl -s $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR) -d $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR) -n $(MOZ_PKG_APPNAME) -v -od -x "$(XPIDL_LINK)"
+endif

I wouldn't bother with xptlink.pl at all here, it doesn't seem like it's adding any value. You can call the xpt_link binary directly, just do something like:
$(NSINSTALL) -D $(DIST)/xpt-temp
$(XPIDL_LINK) $(DIST)/xpt-temp/$(MOZ_PKG_APPNAME).xpt $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)/components/*.xpt
rm -f $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)/components/*.xpt
cp $(DIST)/xpt-temp/$(MOZ_PKG_APPNAME).xpt $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)/components

Comment 30

[(not currently active) Ted Mielczarek]

Comment on attachment 359321 [details] [diff] [review]
xptlinking patch on the 'mobile' side

This isn't quite going to work.

+# Keep packager.mk from copying xulrunner dist over.
+TAR_EXCLUDE_LIST = --exclude="xulrunner"
+

Use NO_PKG_FILES, as I said in the other comment. (I'm not sure why Fennec has to do the silly copying of xulrunner around to make life more difficult anyway.)

+libs::
+	@echo "Packaging Xulrunner..."
+	@$(MAKE) package -C $(LIBXUL_DIST)/..
+	cd $(DIST)/$(MOZ_PKG_DIR); cat $(LIBXUL_DIST)/xulrunner*$(PKG_SUFFIX) | $(UNMAKE_PACKAGE)

This is going to fail if you're building using a xulrunner SDK and not a real objdir. You can check ifdef SYSTEM_LIBXUL to handle this case. I think you'll just have to skip trying to package xulrunner in fennec in that case.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #30)

> +# Keep packager.mk from copying xulrunner dist over.
> +TAR_EXCLUDE_LIST = --exclude="xulrunner"
> +
> 
> Use NO_PKG_FILES, as I said in the other comment. (I'm not sure why Fennec has
> to do the silly copying of xulrunner around to make life more difficult
> anyway.)

We want to change! We can package and unpackage xulrunner

Comment 32

[Frederic Plourde (:fred23)]

Attachment: XPTlinking on the XULrunner side

Ted, mFinkle : 

Ok guys, here it is. Last version of the XPTlink patch on the XULrunner side. Should be consistent with today's discussion.

Comment 33

[Frederic Plourde (:fred23)]

Attachment: XPTlinking on the Mobile side

ted, mFinkle :

and here's the other needed patch.. on the 'mobile' side.

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 363795 [details] [diff] [review]
XPTlinking on the Mobile side

This should handle the | make deb | step, I assume. We will need a similar chunk of code for windows mobile too for | make cab |

Is there an earlier phase we can put the code so we don't duplicate it?

Perhaps we could look into that in a followup bug.

Comment 35

[Frederic Plourde (:fred23)]

(In reply to comment #34)
> (From update of attachment 363795 [details] [diff] [review])
> This should handle the | make deb | step, I assume.

yes, I checked. 'make deb' launches the regular make on mobile/installer, prior to making the deb stuff. So everything gets done properly.

> We will need a similar
> chunk of code for windows mobile too for | make cab |

I admit I'm not sure about that one. Do we have time to correct that for this evening's code freeze ?.. you tell me.

Comment 36

[Frederic Plourde (:fred23)]

Attachment: XPT linking v.4 for M-C

This patch MUST be applied on top of wolfe's patches for Dynamic WinCE CAB INF File Production (Bug 476733)... on the Mozilla-central side.

Comment 37

[Frederic Plourde (:fred23)]

Attachment: XPT linking v.4 for m-b

This patch MUST be applied on top of wolfe's patches for Dynamic WinCE CAB INF
File Production (Bug 476733)... on the mobile-browser side.

Comment 38

[(not currently active) Ted Mielczarek]

Comment on attachment 365200 [details] [diff] [review]
XPT linking v.4 for M-C

>diff --git a/config/autoconf.mk.in b/config/autoconf.mk.in
>+SYSTEM_LIBXUL   = @SYSTEM_LIBXUL@

I can't believe this didn't get committed with the patch that added it. WTF?

>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>@@ -407,7 +407,16 @@ ifdef MOZ_PKG_REMOVALS
>     $(SYSINSTALL) $(IFLAGS1) $(MOZ_PKG_REMOVALS_GEN) $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)
> endif # MOZ_PKG_REMOVALS
> 
>-make-package: stage-package
>+ifdef LINK_XPT_WITHOUT_MANIFEST

Remove the blank line here.

>+    @echo "Linking XPT files..."
>+    $(RM) -rf $(DIST)/xpt

Just use rm here, not $(RM). Also, you should preface all your commands with @, like the rest of the commands in this rule, so make doesn't echo them.

I wonder if it doesn't make sense to just get rid of this LINK_XPT_WITHOUT_MANIFEST variable, and unconditionally link the xpt files in the !MOZ_PKG_MANIFEST case? There's no downside to it, AFAICT, and it's one less makefile variable to deal with. I think you should just stick all your new bits right before the endif #MOZ_PKG_MANIFEST here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#382

But overall the patch looks good. Just make that change and it's r+ for sure.

Comment 39

[(not currently active) Ted Mielczarek]

Comment on attachment 365202 [details] [diff] [review]
XPT linking v.4 for m-b

>diff --git a/installer/Makefile.in b/installer/Makefile.in
>+ifndef SYSTEM_LIBXUL
>+NO_PKG_FILES += xulrunner $(NULL)
>+endif

Ditch the $(NULL). It's literally just an undefined variable, we only use it for ending multi-line variable values with continuations.

>+PACKAGE_XUL = packageXUL

This doesn't seem to be used anywhere.

>+packageXUL:

We generally make our (non-filename) makefile targets all lowercase and dash separated. I would use "package-xulrunner" here.

>+ifndef SYSTEM_LIBXUL
>+    @echo "Packaging XULrunner..."
>+    @$(MAKE) package -C $(LIBXUL_DIST)/.. || echo "Perhaps you're trying to package a prebuilt SDK. See 'https://wiki.mozilla.org/Mobile/Build/Fennec#Build' for more information."

I'd like the makefile target to be after the directory name, makes it easier for me to read:
@$(MAKE) -C $(LIBXUL_DIST)/.. package || ...

Very nice error message!

>+    cd $(DIST)/$(MOZ_PKG_DIR); cat $(LIBXUL_DIST)/*$(PKG_SUFFIX) | $(UNMAKE_PACKAGE)

I'm not wild about the *$(PKG_SUFFIX) here, but I guess you don't actually know the package name. I'd prefer to see it a little more narrowed down though.

>+else
>+    @echo "Using SYSTEM libXUL..."
>+endif

"Using system xulrunner"
>-cab: stage-package
>+UNPACKAGE = $(LIBXUL_DIST)/*$(PKG_SUFFIX)
>+cab: stage-package packageXUL

You're going to have to merge with John's patches on bug 476733. Also, I don't think the way you have this written will handle making "package" as opposed to "deb/cab". I think you can just do:
make-package deb cab: package-xulrunner

r+ with those fixes.

Comment 40

[Phil Ringnalda (:philor)]

(In reply to comment #38)
> unconditionally link the xpt files in
> the !MOZ_PKG_MANIFEST case? There's no downside to it, AFAICT

Do it. It'll be very slightly awkward for Thunderbird to ifdef the removals of the unlinked ones for only 1.9.2, until it lands on 1.9.1 (if it's going to), but after three months I just want to see *something* happen, I no longer care what.

Comment 41

[(not currently active) Ted Mielczarek]

I would just ignore trunk for Thunderbird for a short time after this lands. We'll get it on 1.9.1 ASAP, since mobile wants to ship from there.

Comment 42

[Frederic Plourde (:fred23)]

Attachment: XPT linking v.5 for M-C (must apply ontop of associated Wolfe's patch Bug #476733)

(In reply to comment #38)
> I wonder if it doesn't make sense to just get rid of this
> LINK_XPT_WITHOUT_MANIFEST variable, and unconditionally link the xpt files in
> the !MOZ_PKG_MANIFEST case?

nah... we've gotta keep that variable because it prevents m-b from linking its XPT files (which it can't anyway). see ? Since packager.mk is linked from within both mobile and xulrunner apps, we have to be able to assign those commands to xulrunner only. You can try it. remove the ifdef and | make package | is going to crash for sure.

Comment 43

[Frederic Plourde (:fred23)]

Attachment: XPT linking v.5 for m-b (must apply ontop of associated Wolfe's patch Bug #476733)

(In reply to comment #39)
> you're going to have to merge with John's patches on bug 476733. Also, I don't
> think the way you have this written will handle making "package" as opposed to
> "deb/cab". I think you can just do:

already done. The present patch applies on top of Wolfe's patch. Everything should run smoothly ;) for every targets and platforms.

Comment 44

[(not currently active) Ted Mielczarek]

(In reply to comment #42)
> nah... we've gotta keep that variable because it prevents m-b from linking its
> XPT files (which it can't anyway). see ? Since packager.mk is linked from
> within both mobile and xulrunner apps, we have to be able to assign those
> commands to xulrunner only. You can try it. remove the ifdef and | make package
> | is going to crash for sure.

I'm sorry, I don't get what you mean here. Why will this be a problem on fennec?

Comment 45

[Frederic Plourde (:fred23)]

(In reply to comment #44)

> I'm sorry, I don't get what you mean here. Why will this be a problem on
> fennec?

The "ifdef LINK_XPT_WITHOUT_MANIFEST" is important simply because when you run the | make package | command from objdir/mobile directory, you'll end-up having fennec (mobile) execute those commands (see below) instead of xulrunner only. The @$(XPIDL_LINK) command is what's causing troubles... 

+ifdef LINK_XPT_WITHOUT_MANIFEST
+	@echo "Linking XPT files..."
+	@rm -rf $(DIST)/xpt
+	@$(NSINSTALL) -D $(DIST)/xpt
+	@$(XPIDL_LINK) $(DIST)/xpt/$(MOZ_PKG_APPNAME).xpt $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)/components/*.xpt
+	@rm -f $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)/components/*.xpt
+	@cp $(DIST)/xpt/$(MOZ_PKG_APPNAME).xpt $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)/components
+endif


see, when executed from the mobile side, this command becomes : objdir/xulrunner/dist/bin/xpt_link ../../dist/xpt/fennec.xpt ../../dist/fennec/components/*.xpt

I get this ERROR:
FAILED: get_file_length: No such file or directory

because there's no single .xpt file in the dist/fennec/components/ dir... All of this is supposed to be done on the xulrunner side only, hence the abovementionned "ifdef".

As you see, we're kind of stuck with this ifdef because I definitely think that we should keep the XPTLINK chunk of code in packager.mk.

Comment 46

[(not currently active) Ted Mielczarek]

We should just fix that to not error if there are no xpt files, then. Something like:
ifneq(,$(wildcard $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)/components/*.xpt)
    @echo "Linking XPT files..."
endif

Comment 47

[Frederic Plourde (:fred23)]

Attachment: XPT linking v.6 for M-C (must apply ontop of associated Wolfe's v5.0 patch Bug #476733) [Checkin: Comment 52]

(In reply to comment #46)

> We should just fix that to not error if there are no xpt files, then.
> Something like:
> ifneq(,$(wildcard $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)/components/*.xpt)
>     @echo "Linking XPT files..."
> endif

A very nice solution indeed.  It's been included in the proposed patch and it runs without a glitch. thanx !

Comment 48

[(not currently active) Ted Mielczarek]

Comment on attachment 370275 [details] [diff] [review]
XPT linking v.6 for M-C (must apply ontop of associated Wolfe's v5.0 patch Bug #476733)
[Checkin: Comment 52]

Thanks, looks good!

Comment 49

[Frederic Plourde (:fred23)]

Attachment: commit information for XPT linking patch v5.0 for m-b

Since Wolfe's patches landed, here's commit information for the m-b patch

Comment 50

[Frederic Plourde (:fred23)]

Attachment: Commit information for XPT linking v.6 for M-C

Since Wolfe's patches landed, here's commit information for the M-C patch v6.0

Comment 51

[Frederic Plourde (:fred23)]

Attachment: XPT linking v.6 for m-b (must apply ontop of associated Wolfe's patch Bug #476733) [Checkin: Comment 52]

There was only a small change to make in this patch in order for it to apply nicely... because wolfe's m-b patch underwent a tiny change just before landing. Use THIS one (v6.0) for m-b instead of obsolete v.5

Comment 52

[Brad Lassey [:blassey] (use needinfo?)]

pushed
http://hg.mozilla.org/mozilla-central/rev/29867aa6a022
and
http://hg.mozilla.org/mobile-browser/rev/323cf18c81ad

Comment 53

[Frederic Plourde (:fred23)]

(In reply to comment #52)
> pushed
> http://hg.mozilla.org/mozilla-central/rev/29867aa6a022
> and
> http://hg.mozilla.org/mobile-browser/rev/323cf18c81ad

Thanks Brad!...

but I think the commit info on the m-b side got screwed up.
The hg log reads : 
"Brad Lassey - imported patch xptlinkv6.patch" instead of showing bug number, reviewer and author correctly like on the M-C side, 
where it reads :
"Frederic Plourde - Bug 469873 - Fennec building does not link XPT files in a single bundle : no call to xptlink.pl; r+ted"

just to let you know... in case that could be changed.

Comment 54

[Mark Finkle (:mfinkle) (use needinfo?)]

Looks like we got our Ts speedup from this patch:
http://graphs.mozilla.org/#show=2774468,2774466,2774464&sel=1239158895,1239233216

Thanks Frederic and Ted!

Comment 55

[John Wolfe (:wolfe)]

Attachment 371675 [details] [diff] changed the "make installer" command desired by ted into a "make cab" command that also made the package (a ZIP file) unnecessarily.

Comment 56

[John Wolfe (:wolfe)]

Attachment: fixup for reverting "make cab" command into the desired "make installer" command

I think ted might have missed the change to mobile/installer/makefile.in which changed the "make installer" command into a modified "make cab" command.

This small patch just changes the makefile command back to "make installer"

Comment 57

[John Wolfe (:wolfe)]

Comment on attachment 371900 [details] [diff] [review]
fixup for reverting "make cab" command into the desired "make installer" command

oops - review to ted

Comment 58

[John Wolfe (:wolfe)]

Attachment: revised fixup - put back "package-xulrunner" dependency, still changes "make cab" into "make installer" [Checkin: Comment 59]

I had not realized all the extra work that had gone into the patch that landed.

In order to have "make installer" work on WinCE / WinMobile builds, the "package-xulrunner" dependency needs to be in place.

Unfortunately, this means making a CAB does two copies of all the files needed for a fennec build, PLUS a ZIP and a UNZIP operation.  SIGH.  I am sure that there is some deep reason for the extra work, but I do not understand it.

Since I do not understand why xulrunner directory is removed, I am restoring the "package-xulrunner" dependency that I removed with my previous attachment on this bug.

Comment 59

[Aki Sasaki (not active)]

Comment on attachment 371922 [details] [diff] [review]
revised fixup - put back "package-xulrunner" dependency, still changes "make cab" into "make installer"
[Checkin: Comment 59]

rev 473|5b12701b901b

Comment 60

[Frederic Plourde (:fred23)]

Thanks Wolfe !  I had not realized this 'installer' target had switched to a 'cab' one.

Comment 61

[Frederic Plourde (:fred23)]

Wolfe: 
I re-pulled the whole tree and built from tip for wince and noticed weird things with wince installer.

I think that might be affected by the recent patches of yours in bug 476733, namely, the m-b v5.1 and m-c v6.2. There are quite a lot of stuff there.

I think the whole issue might be related to an error I get :
" MOZ_PKG_CAB_SCRIPT is not defined ! ", which keeps xulrunner from packagin properly and hence, results in a xulrunner-less fennec .cab file.

more info coming up, possibly with a patch.

Comment 62

[John Wolfe (:wolfe)]

I do not see this issue you are seeing - caused because of a line in packager.mk that was removed with the landing of bug 476733'a m-c patch.

There is no MOZ_PKG_CAB_SCRIPT string anywhere in the mrx.mozilla.org for mozilla-central or mobile-browser.  

Are you sure that your pull succeeded correctly?

Comment 63

[Frederic Plourde (:fred23)]

Wolfe: 

 ok , indeed, there was something screwed up in the pull process. I'm not sure yet what that was, but it was definitely on my side. Now, no more MOZ_PKG_CAB_SCRIPT error, but XPT files still get copied over to final CAB (or fennec ZIP) for wince builds.

My guess is that the NO_PKG_FILES=xulrunner is not kickin' in as it should. I unfortunately haven't had enough time to investigate throughly because it's Easter weekend and it gets celebrated quite a lot here (family gatherings) in this part of Canada.. So I'll be back at work next tuesday to finish that story.

IMO, it's really minor issue and it should get fixed for wince in no time.

Comment 64

[Frederic Plourde (:fred23)]

Attachment: a quick patchup to packager.mk for fixing $(wildcard) not being evaluated at runtime

Ted:
Here's a small patch in response to what we've found concerning the "wildcard" command not being evaluated at runtime and thus, making XPT linking not kick in properly in non-xpt-folders cases.

After that, we shall re-close the current bug.

Comment 65

[(not currently active) Ted Mielczarek]

Comment on attachment 372891 [details] [diff] [review]
a quick patchup to packager.mk for fixing $(wildcard) not being evaluated at runtime

Kind of sucks, but that's make for you...

Comment 66

[Frederic Plourde (:fred23)]

Attachment: export patch with user and commit message [Checkin: Comment 67]

Got reviewed. Ready for commit.
This exported patch has the user and commit message in it

Comment 67

[Doug Turner (:dougt)]

Comment on attachment 374302 [details] [diff] [review]
export patch with user and commit message
[Checkin: Comment 67]

http://hg.mozilla.org/mozilla-central/rev/865d6a8f4984

Comment 68

[Frederic Plourde (:fred23)]

Attachment: Unit test for XPT_LINK

A small C standalone test program to see if XPT_LINK is doing fine.
It should be fairly complete, but it could use some review.
thanks.

Comment 69

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

Comment on attachment 374302 [details] [diff] [review]
export patch with user and commit message
[Checkin: Comment 67]

a191=beltzner

Comment 70

[(not currently active) Ted Mielczarek]

Comment on attachment 375696 [details] [diff] [review]
Unit test for XPT_LINK

I'd rather see this as a Python program or even a shell script, or even just as raw make commands under check::. There's no reason it needs to be a C++ program.

Comment 71

[Phil Ringnalda (:philor)]

Attachment: Don't try to link with the wrong path on OS X [Checkin: Comment 76]

So, that patch, since it actually works, does exactly what I was talking about in comment 3 - apparently for a month now we've been successfully linking XPTs in Thunderbird trunk on Linux, and thus for people using the updater having both new versions of interfaces in thunderbird.xpt and the old ones in the separate XPTs.

Fortunately, if only accidentally, you aren't breaking Firefox on OS X the same way - the Firefox Mac tinderbox logs now show

Linking XPT files...
FAILED: get_file_length: No such file or directory
No *.xpt files found in: ../../dist/firefox/components/. Continuing...

which is a damn good thing, but rather than depending on the vars giving you the wrong directory so you don't break Mac, the whole linking chunk should be up above the |endif # DMG|.

Comment 72

[Phil Ringnalda (:philor)]

Comment on attachment 374302 [details] [diff] [review]
export patch with user and commit message
[Checkin: Comment 67]

You can't land this on 1.9.1, since it applies on top of attachment 370275 [details] [diff] [review], which you didn't land on 1.9.1. Rollup patch of all three in a second...

Comment 73

[Phil Ringnalda (:philor)]

Attachment: 1.9.1 rollup patch

This is the combination of attachment 374302 [details] [diff] [review] (r=ted, was a=beltzner but couldn't actually apply), attachment 370275 [details] [diff] [review] (r=ted), and attachment 379484 [details] [diff] [review] (still r?=ted, so obviously I won't land it until that's reviewed and cycled on m-c).

Comment 74

[Phil Ringnalda (:philor)]

Outside the question of unit testing (which you don't have now, anyway), testing this in terms of Firefox requires just four things, all of which I plan to do anywhere I get to land any part of it:

* Download a Linux build, untar it, verify that the components/ directory still contains a "firefox.xpt" and doesn't contain any other files named *.xpt, just like always

* Download a Windows build, install/unzip it, verify that the components/ directory contains a "firefox.xpt" and doesn't contain any other files named *.xpt, just like always

* Download a Mac build, open the dimmage, show package contents, verify that the components/ directory does not contain a "firefox.xpt" and does contain many other files named *.xpt, just like always

* Open the full build log for the Mac build, cmd+F, "linking xpt files", verify that nothing is found, like it used to be

(Verifying that it actually does what Fennec wants I consider outside my department, but I did verify that the 1.9.1 rollup links xpts for Thunderbird/Linux, which also lacks a MOZ_PKG_MANIFEST, so... it probably does.)

Comment 75

[(not currently active) Ted Mielczarek]

Comment on attachment 379484 [details] [diff] [review]
Don't try to link with the wrong path on OS X
[Checkin: Comment 76]

Good catch. Must have slipped past me in one of the zillion versions of this patch.

Comment 76

[Phil Ringnalda (:philor)]

Comment on attachment 379484 [details] [diff] [review]
Don't try to link with the wrong path on OS X
[Checkin: Comment 76]

http://hg.mozilla.org/mozilla-central/rev/27e98aed475d

Comment 77

[Phil Ringnalda (:philor)]

(In reply to comment #74)
> verify that the components/ directory still
> contains a "firefox.xpt" and doesn't contain any other files named *.xpt, just
> like always

Err, it's browser.xpt, just like always, but the ones that should be linked still are, and the ones that shouldn't be aren't and aren't trying anymore.

Comment 78

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

Comment on attachment 379496 [details] [diff] [review]
1.9.1 rollup patch

We're closed for changes to 1.9.1. Please nominate for wanted1.9.1.x? if you want to see this in 1.9.1.1 (which will be used for Fennec)

Comment 79

[Serge Gautherie (:sgautherie)]

What is left to do on this bug?

Comment 80

[Phil Ringnalda (:philor)]

Someone needs to add the DONTEVERREOPENTHISAGAIN resolution.