Last Comment Bug 309210 - makexpi.pl only looks for *.so when stripping
: makexpi.pl only looks for *.so when stripping
Status: RESOLVED FIXED
[nvn-dl]
: fixed1.8.0.2, fixed1.8.1
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Doron Rosenberg (IBM)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-19 15:24 PDT by jhp (no longer active)
Modified: 2006-03-15 06:51 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.51 KB, patch)
2005-12-12 15:01 PST, Doron Rosenberg (IBM)
benjamin: review-
Details | Diff | Splinter Review
oops, missed the target (1.13 KB, patch)
2005-12-19 13:17 PST, Doron Rosenberg (IBM)
benjamin: review+
dveditz: approval1.8.0.1-
Details | Diff | Splinter Review
STRIP_XPI patch (1.10 KB, patch)
2006-02-06 10:13 PST, Doron Rosenberg (IBM)
benjamin: review+
benjamin: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2-
Details | Diff | Splinter Review
1.8.0 branch version (1.75 KB, patch)
2006-02-22 14:10 PST, Doron Rosenberg (IBM)
benjamin: review+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review

Description jhp (no longer active) 2005-09-19 15:24:04 PDT
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) 2005-10-07 08:23:15 PDT
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) 2005-11-30 11:10:18 PST
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) 2005-11-30 11:16:06 PST
bsmedberg doron: gcc includes symbols by default
Comment 4 Doron Rosenberg (IBM) 2005-12-12 15:01:20 PST
Created attachment 205665 [details] [diff] [review]
patch

Seems to work fine on linux, needs mac testing before I request reviews.
Comment 5 jhp (no longer active) 2005-12-15 13:14:23 PST
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 [:bsmedberg] 2005-12-19 11:44:36 PST
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) 2005-12-19 13:17:05 PST
Created attachment 206323 [details] [diff] [review]
oops, missed the target

Indeed, should be in the target
Comment 8 Benjamin Smedberg [:bsmedberg] 2005-12-19 13:20:08 PST
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 [:bz] (still a bit busy) 2005-12-31 12:11:53 PST
So does this need to get landed?
Comment 10 Doron Rosenberg (IBM) 2005-12-31 12:32:49 PST
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) 2006-01-03 11:09:29 PST
checked into trunk
Comment 12 Doron Rosenberg (IBM) 2006-01-03 11:25:54 PST
Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

Would like this for the 2 branches.
Comment 13 Daniel Veditz [:dveditz] 2006-01-05 12:13:59 PST
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 2006-01-05 15:40:12 PST
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 2006-01-05 17:40:04 PST
I'm getting the same error as Comment #14 with a Firefox build on Win32/MingW/cygwin
Comment 16 Doron Rosenberg (IBM) 2006-01-05 20:02:38 PST
(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 2006-01-05 20:20:23 PST
(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 2006-01-05 20:58:57 PST
I can also confirm failed builds using MSVC 7.1 on Win32. Same exact error as posted above.
Comment 19 Doron Rosenberg (IBM) 2006-01-06 07:57:20 PST
(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) 2006-01-06 08:01:43 PST
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 2006-01-06 08:58:24 PST
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 2006-01-06 13:50:31 PST
(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 [:bsmedberg] 2006-01-06 13:52:38 PST
The point was not to fix it, but to allow you/us to see what command was actually being run...
Comment 24 Philip Puryear 2006-01-06 14:20:30 PST
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 2006-01-07 11:59:49 PST
I get the same with MSVC6 WinXP, (doesnt that mean this patch breaks a TIER1 platform....)
Comment 26 Bernd 2006-01-07 12:15:50 PST
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) 2006-01-07 12:17:54 PST
(bangbang023@optonline.net: 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) 2006-01-07 13:32:14 PST
(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) 2006-01-07 13:59:40 PST
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) 2006-01-07 14:11:37 PST
suggested on irc:

what does |which find| return for you?
Comment 31 Philip Puryear 2006-01-07 15:56:16 PST
(In reply to comment #30)
> suggested on irc:
> 
> what does |which find| return for you?
> 

/cygdrive/c/WINDOWS/system32/find
Comment 32 Benjamin Smedberg [:bsmedberg] 2006-01-07 18:43:10 PST
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) 2006-01-07 18:53:32 PST
(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 2006-01-07 18:58:36 PST
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 2006-01-09 20:44:27 PST
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 [:bsmedberg] 2006-01-30 11:58:18 PST
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) 2006-01-30 12:31:08 PST
I just submitted a stack (under doronr@gmail.com), will take a few days to be processed though.
Comment 38 timeless 2006-01-30 12:32:08 PST
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) 2006-02-02 12:11:59 PST
(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) 2006-02-03 09:35:17 PST
(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) 2006-02-06 07:22:10 PST
(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 [:bsmedberg] 2006-02-06 07:32:02 PST
How about ifdef STRIP_XPI ? And only set STRIP_XPI for xforms/lightning?
Comment 43 timeless 2006-02-06 09:07:15 PST
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) 2006-02-06 10:13:47 PST
Created attachment 210896 [details] [diff] [review]
STRIP_XPI patch
Comment 45 Benjamin Smedberg [:bsmedberg] 2006-02-06 10:38:18 PST
Comment on attachment 210896 [details] [diff] [review]
STRIP_XPI patch

This should be safe for the branches also.
Comment 46 Doron Rosenberg (IBM) 2006-02-06 11:09:08 PST
Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

obseleting
Comment 47 Doron Rosenberg (IBM) 2006-02-06 12:58:05 PST
checked into 1.8.1
Comment 48 Chase Phillips 2006-02-07 11:48:07 PST
Mass reassign of open bugs for chase@mozilla.org to build@mozilla-org.bugs.
Comment 49 Daniel Veditz [:dveditz] 2006-02-22 12:04:09 PST
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] 2006-02-22 12:05:36 PST
Comment on attachment 210896 [details] [diff] [review]
STRIP_XPI patch

global build/config changes scary for 1.8.0.x
Comment 51 Benjamin Smedberg [:bsmedberg] 2006-02-22 12:32:58 PST
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) 2006-02-22 14:10:19 PST
Created attachment 212809 [details] [diff] [review]
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] 2006-02-23 11:57:04 PST
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) 2006-02-23 14:23:49 PST
thanks for approval, checked in.
Comment 55 Dave Liebreich [:davel] 2006-02-24 10:15:17 PST
moving keywords from whiteboard to keywrods
Comment 56 Allan Beaufour 2006-03-15 05:34:12 PST
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) 2006-03-15 06:51:49 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.