309210 - makexpi.pl only looks for *.so when stripping

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[jhp (no longer active)]

I was looking into why the xforms.xpi is 2M on Mac OS X, but about 200K on Win32
and Linux.  It looks like strip is not getting run on the *.dylib when creating
the xpi.

In 'makexpi.pl', the function |RecursiveStrip| calls "strip" on what is returned
by |@libraryList|.  But that latter function only looks for *.so, not *.dylib. 
Also, |RecursiveStrip| calls strip without any params, which does nothing on Mac
OS X.  I ran "strip -x -S" manually on 'libxforms.dylib' &
'libschemavalidation.dylib', and it greatly reduced their size.

Comment 1

[jhp (no longer active)]

Just tried doing "strip -x -S" on the libs in xforms.xpi, but that rendered
XForms useless.  Looks like the "-x" is too much, and we should only strip the
debug info by doing "strip -S".  Using this, the xforms.xpi goes from 2M to less
than 200K.

Comment 2

[Doron Rosenberg (IBM)]

bsmedberg doron: I bet there isn't any stripping happening at all
bsmedberg doron: in release builds windows DLLs don't have debugging info
bsmedberg doron: the XPI is made at http://lxr.mozilla.org/mozilla/source/config/rules.mk#1552
bsmedberg doron: and you'd probably have to copy or rearrange code from http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/installer/packager.mk#274

Comment 3

[Doron Rosenberg (IBM)]

bsmedberg doron: gcc includes symbols by default

Comment 4

[Doron Rosenberg (IBM)]

Attachment: patch

Seems to work fine on linux, needs mac testing before I request reviews.

Comment 5

[jhp (no longer active)]

Great!  Shrunk the xforms xpi by about 89% on Mac OS X, and the inspector xpi by about 80%.  Installed xforms.xpi and in limited testing, works well.

Comment 6

[Benjamin Smedberg]

Comment on attachment 205665 [details] [diff] [review]
patch

I don't get it... these are rules, but they don't live inside a target. Did you mean to put these rules in the libs realchrome:: target below (Packaging $(XPI_PKGNAME))? This looks like the right place for this stuff.

Comment 7

[Doron Rosenberg (IBM)]

Attachment: oops, missed the target

Indeed, should be in the target

Comment 8

[Benjamin Smedberg]

Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

cd $(FINAL_TARGET); find

should be 

cd $(FINAL_TARGET) && find

Extra whitespace lines are bad in rules (before ifdef/after endif), please remove them.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So does this need to get landed?

Comment 10

[Doron Rosenberg (IBM)]

I was going to land it when I get back to Austin next week, anyone is free to check it in before then :)

Comment 11

[Doron Rosenberg (IBM)]

checked into trunk

Comment 12

[Doron Rosenberg (IBM)]

Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

Would like this for the 2 branches.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

Since this could potentially affect talkback we want more baking, and that probably effectively pushes this into 1.8.0.2

Comment 14

[Philip Puryear]

BlueFyre and I (and possibly bangbang023) have been getting this build error since this was checked in:

> Stripping inspector-1.6a1 package directory...
> ../../dist/xpi-stage/inspector
> make[5]: *** [libs] Error 2
> make[5]: Leaving directory `/cygdrive/c/mozilla/mozilla/extensions/inspector'
> make[4]: *** [libs] Error 2
> make[4]: Leaving directory `/cygdrive/c/mozilla/mozilla/extensions'
> make[3]: *** [libs_tier_94] Error 2
> make[3]: Leaving directory `/cygdrive/c/mozilla/mozilla'
> make[2]: *** [tier_94] Error 2
> make[2]: Leaving directory `/cygdrive/c/mozilla/mozilla'
> make[1]: *** [default] Error 2
> make[1]: Leaving directory `/cygdrive/c/mozilla/mozilla'
> make: *** [build] Error 2

Could this bug be the cause? I've nuked my tree and still get this same error.

Comment 15

[David G King]

I'm getting the same error as Comment #14 with a Firefox build on Win32/MingW/cygwin

Comment 16

[Doron Rosenberg (IBM)]

(In reply to comment #14)
> BlueFyre and I (and possibly bangbang023) have been getting this build error
> since this was checked in:
> 
what os/compiler?

Comment 17

[Philip Puryear]

(In reply to comment #16)
> (In reply to comment #14)
> > BlueFyre and I (and possibly bangbang023) have been getting this build error
> > since this was checked in:
> > 
> what os/compiler?
> 

Win32/MSVC 7.1

Comment 18

[Christopher Vendemio]

I can also confirm failed builds using MSVC 7.1 on Win32. Same exact error as posted above.

Comment 19

[Doron Rosenberg (IBM)]

(In reply to comment #18)
> I can also confirm failed builds using MSVC 7.1 on Win32. Same exact error as
> posted above.
> 

Can you post a .mozconfig you are using, I can't repro on win32 and msvc 7.1 debug

Comment 20

[Doron Rosenberg (IBM)]

Can you try removing the @ from this line:

@cd $(FINAL_TARGET) && find . ! -type d \

in the patched file and see if that works?

Also, what is $(STRIP) and $(STRIP_FLAGS) set to in your systems?

Comment 21

[Christopher Vendemio]

My .mozconfig is as follows. A little older than some. 

. $topsrcdir/browser/config/mozconfig

GLIB_PREFIX=J:/mozilla/w32build/vc7
LIBIDL_PREFIX=J:/mozilla/w32build/vc7

export MOZ_PHOENIX=1
export MOZ_OPTIMIZE_LDFLAGS="-opt:ref,icf,nowin98"

mk_add_options MOZ_PHOENIX=1
mk_add_options MOZ_OPTIMIZE_LDFLAGS="-opt:ref,icf,nowin98"

ac_add_options --enable-canvas
ac_add_options --enable-single-profile
ac_add_options --enable-static
ac_add_options --enable-strip
ac_add_options --enable-svg
ac_add_options --enable-optimize="-O2 -GAL7 -arch:SSE2"

ac_add_options --disable-activex
ac_add_options --disable-activex-scripting
ac_add_options --disable-tests
ac_add_options --disable-debug
ac_add_options --disable-mailnews
ac_add_options --disable-composer
ac_add_options --disable-ldap
ac_add_options --disable-profilesharing
ac_add_options --disable-shared
ac_add_options --disable-accessibility
ac_add_options --disable-installer

Comment 22

[Christopher Vendemio]

(In reply to comment #20)
> Can you try removing the @ from this line:
> > @cd $(FINAL_TARGET) && find . ! -type d \
> > in the patched file and see if that works?

Didn't fix it.

Comment 23

[Benjamin Smedberg]

The point was not to fix it, but to allow you/us to see what command was actually being run...

Comment 24

[Philip Puryear]

The following is the output produced with cd instead of @cd:

>   adding: content/inspector/contents.rdf (stored 0%)
>   adding: skin/classic/inspector/contents.rdf (stored 0%)
>   adding: skin/modern/inspector/contents.rdf (stored 0%)
> Stripping inspector-1.6a1 package directory...
> ../../dist/xpi-stage/inspector
> cd ../../dist/xpi-stage/inspector && find . ! -type d \
> 		! -name "*.js" \
> 		! -name "*.xpt" \
> 		! -name "*.gif" \
> 		! -name "*.jpg" \
> 		! -name "*.png" \
> 		! -name "*.xpm" \
> 		! -name "*.txt" \
> 		! -name "*.rdf" \
> 		! -name "*.sh" \
> 		! -name "*.properties" \
> 		! -name "*.dtd" \
> 		! -name "*.html" \
> 		! -name "*.xul" \
> 		! -name "*.css" \
> 		! -name "*.xml" \
> 		! -name "*.jar" \
> 		! -name "*.dat" \
> 		! -name "*.tbl" \
> 		! -name "*.src" \
> 		! -name "*.reg" \
> 		 \
> 		-exec echo not_strip  {} >/dev/null 2>&1 \;
> make[5]: Leaving directory `/cygdrive/c/builds/trees/browser_trunk/mozilla/objdir/extensions/inspector'
> make[4]: Leaving directory `/cygdrive/c/builds/trees/browser_trunk/mozilla/objdir/extensions'
> make[3]: Leaving directory `/cygdrive/c/builds/trees/browser_trunk/mozilla/objdir'
> make[2]: Leaving directory `/cygdrive/c/builds/trees/browser_trunk/mozilla/objdir'
> make[1]: Leaving directory `/cygdrive/c/builds/trees/browser_trunk/mozilla/objdir'

Comment 25

[Bernd]

I get the same with MSVC6 WinXP, (doesnt that mean this patch breaks a TIER1 platform....)

Comment 26

[Bernd]

hmm it would probably be cool if at least one win ff tinderbox would build the stuff that we ship like inspector.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

([e-mail]: I would really not suggest these optimize flag (specifically, the opt:icf one). see http://www.mozilla.org/js/spidermonkey/release-notes/NOICF.html)

Comment 28

[Doron Rosenberg (IBM)]

(In reply to comment #25)
> I get the same with MSVC6 WinXP, (doesnt that mean this patch breaks a TIER1
> platform....)
> 

Going to take a look.  If we can't figure this out soon, yeah, we should back out.

Comment 29

[Doron Rosenberg (IBM)]

Windows debug build with vc6:

Stripping inspector-1.6a1 package directory...
../../dist/xpi-stage/inspector
cd ../../dist/xpi-stage/inspector && find . ! -type d \
                ! -name "*.js" \
                ! -name "*.xpt" \
                ! -name "*.gif" \
                ! -name "*.jpg" \
                ! -name "*.png" \
                ! -name "*.xpm" \
                ! -name "*.txt" \
                ! -name "*.rdf" \
                ! -name "*.sh" \
                ! -name "*.properties" \
                ! -name "*.dtd" \
                ! -name "*.html" \
                ! -name "*.xul" \
                ! -name "*.css" \
                ! -name "*.xml" \
                ! -name "*.jar" \
                ! -name "*.dat" \
                ! -name "*.tbl" \
                ! -name "*.src" \
                ! -name "*.reg" \
                 \
                -exec echo not_strip  {} >/dev/null 2>&1 \;
Packaging inspector-1.6a1.xpi...


No error or anything, hmm.

Comment 30

[Doron Rosenberg (IBM)]

suggested on irc:

what does |which find| return for you?

Comment 31

[Philip Puryear]

(In reply to comment #30)
> suggested on irc:
> 
> what does |which find| return for you?
> 

/cygdrive/c/WINDOWS/system32/find

Comment 32

[Benjamin Smedberg]

That's bad. I'm surprised your build configures, even, and it certainly wouldn't package properly. I'm tempted to mark this FIXED again and update the build docs that "find" must be the unix-style find and not the windows "find" program, which is totally different.

Comment 33

[Doron Rosenberg (IBM)]

(In reply to comment #32)
> That's bad. I'm surprised your build configures, even, and it certainly
> wouldn't package properly. I'm tempted to mark this FIXED again and update the
> build docs that "find" must be the unix-style find and not the windows "find"
> program, which is totally different.
> 

Could we make configure test to see if find is the cygwin one on windows?  Or is that frowned upon?

Comment 34

[Philip Puryear]

I checked the devmo docs about an hour ago and noticed that someone had added an example build script to the Windows section that noted that cygwin\bin must precede the default path so as to override Windows' find tool. It must have been recently added because when I last read the build docs (about six months ago), there was no such warning. Oh well, better late than never, I guess...

Comment 35

[jpl24]

I believe using the sample VC 7.1 file on devmo at http://developer.mozilla.org/en/docs/Windows_Build_Prerequisites will result in this problem, specifically the 3rd line of the "clean slate start". The VC6 example doesn't have this problem. That example should probably be updated. 

Comment 36

[Benjamin Smedberg]

Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

I'd like to see some kind of verification that this did/didn't affect the talkback symbols for DOMI in Firefox. Is there a way to force-crash DOMI?

Comment 37

[Doron Rosenberg (IBM)]

I just submitted a stack (under [e-mail]), will take a few days to be processed though.

Comment 38

[timeless]

so erm, are we intentionally hosing apple's crash reporter, and sampler applications, or am i missing something?

in short, please don't do this.

Comment 39

[Doron Rosenberg (IBM)]

(In reply to comment #38)
> so erm, are we intentionally hosing apple's crash reporter, and sampler
> applications, or am i missing something?
> 
> in short, please don't do this.
> 

Hosing in what way?

Comment 40

[Doron Rosenberg (IBM)]

(In reply to comment #36)
> (From update of attachment 206323 [details] [diff] [review] [edit])
> I'd like to see some kind of verification that this did/didn't affect the
> talkback symbols for DOMI in Firefox. Is there a way to force-crash DOMI?
> 

Hmm, http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=14671337 is a result on trunk from bug https://bugzilla.mozilla.org/show_bug.cgi?id=315429.

Comment 41

[Doron Rosenberg (IBM)]

(In reply to comment #36)
> (From update of attachment 206323 [details] [diff] [review] [edit])
> I'd like to see some kind of verification that this did/didn't affect the
> talkback symbols for DOMI in Firefox. Is there a way to force-crash DOMI?
> 

Looks like this did cause problems.  Is there a better place to put such stripping code?

Comment 42

[Benjamin Smedberg]

How about ifdef STRIP_XPI ? And only set STRIP_XPI for xforms/lightning?

Comment 43

[timeless]

it means that you can't get any useful information from either apple crash reporter or apple's sampler. we've used sampler for things like the silly long url bug, and there are times when talkback doesn't catch crashes which crashreporter does.

Comment 44

[Doron Rosenberg (IBM)]

Attachment: STRIP_XPI patch

Comment 45

[Benjamin Smedberg]

Comment on attachment 210896 [details] [diff] [review]
STRIP_XPI patch

This should be safe for the branches also.

Comment 46

[Doron Rosenberg (IBM)]

Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

obseleting

Comment 47

[Doron Rosenberg (IBM)]

checked into 1.8.1

Comment 48

[Chase Phillips]

Mass reassign of open bugs for [e-mail] to [e-mail].

Comment 49

[Daniel Veditz [:dveditz]]

Comment on attachment 210896 [details] [diff] [review]
STRIP_XPI patch

>-ifndef PKG_SKIP_STRIP
>+ifdef STRIP_XPI

>Index: extensions/xforms/Makefile.in
>+STRIP_XPI = 1

Prior to this change it looks like we stripped everywhere EXCEPT xulrunner install. Now the only place we'll run this code is in xforms. Can that be right?

Comment 50

[Daniel Veditz [:dveditz]]

Comment on attachment 210896 [details] [diff] [review]
STRIP_XPI patch

global build/config changes scary for 1.8.0.x

Comment 51

[Benjamin Smedberg]

Attachment 210896 [details] [diff] is not a rollup patch: currently the 1.8.0 branch doesn't do stripping at all, and the point of that last patch was to keep it that way by default (so we only strip the xforms XPI). I'll get doron to make a rollup patch for the branch.

Comment 52

[Doron Rosenberg (IBM)]

Attachment: 1.8.0 branch version

Too make this patch clear:

Right now, we only strip XPIs that are shipped by default.  This means that XPIs that some tboxen build but aren't part of the core release (Lighting, XForms) are not being striped on linux/mac.  This patch adds a way for the extension to say "strip my libs".

Comment 53

[Daniel Veditz [:dveditz]]

Comment on attachment 212809 [details] [diff] [review]
1.8.0 branch version

approved for 1.8.0 branch, a=dveditz for drivers

Comment 54

[Doron Rosenberg (IBM)]

thanks for approval, checked in.

Comment 55

[Dave Liebreich [:davel]]

moving keywords from whiteboard to keywrods

Comment 56

[Allan Beaufour]

Hmmm, it sure does strip the XPI.

Problem is at built-in extensions that uses STRIP_XPI will also be installed under dist/bin/extensions in their stripped versions -- even if you are doing a debug build. That's not ideal I think.

Either we (xforms) need to only define STRIP_XPI if != debug build, or it has to be handled globally. I would think that it would make sense to handle it globally. Or?

Comment 57

[Doron Rosenberg (IBM)]

(In reply to comment #56)
> Hmmm, it sure does strip the XPI.
> 
> Problem is at built-in extensions that uses STRIP_XPI will also be installed
> under dist/bin/extensions in their stripped versions -- even if you are doing a
> debug build. That's not ideal I think.
> 
> Either we (xforms) need to only define STRIP_XPI if != debug build, or it has
> to be handled globally. I would think that it would make sense to handle it
> globally. Or?
> 

Bug 329796 is for XForms to set it only during debug.  If you want to do it globally, probably should be done in a different bug.