Closed Bug 309210 Opened 15 years ago Closed 15 years ago

makexpi.pl only looks for *.so when stripping

Categories

(Firefox Build System :: General, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jhpedemonte, Assigned: doronr)

Details

(Keywords: fixed1.8.0.2, fixed1.8.1, Whiteboard: [nvn-dl])

Attachments

(3 files, 1 obsolete file)

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.
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.
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
bsmedberg doron: gcc includes symbols by default
Attached patch patchSplinter Review
Seems to work fine on linux, needs mac testing before I request reviews.
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.
Attachment #205665 - Flags: review?(benjamin)
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.
Attachment #205665 - Flags: review?(benjamin) → review-
Attached patch oops, missed the target (obsolete) — Splinter Review
Indeed, should be in the target
Attachment #206323 - Flags: review?(benjamin)
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.
Attachment #206323 - Flags: review?(benjamin) → review+
So does this need to get landed?
I was going to land it when I get back to Austin next week, anyone is free to check it in before then :)
checked into trunk
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

Would like this for the 2 branches.
Attachment #206323 - Flags: approval1.8.1?
Attachment #206323 - Flags: approval1.8.0.1?
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
Attachment #206323 - Flags: approval1.8.0.1? → approval1.8.0.1-
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.
I'm getting the same error as Comment #14 with a Firefox build on Win32/MingW/cygwin
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
(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
I can also confirm failed builds using MSVC 7.1 on Win32. Same exact error as posted above.
(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
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?
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
(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.

The point was not to fix it, but to allow you/us to see what command was actually being run...
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'
I get the same with MSVC6 WinXP, (doesnt that mean this patch breaks a TIER1 platform....)
hmm it would probably be cool if at least one win ff tinderbox would build the stuff that we ship like inspector.
(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)
(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.
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.
suggested on irc:

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

/cygdrive/c/WINDOWS/system32/find
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.
(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?
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...
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 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?
Attachment #206323 - Flags: approval1.8.1? → branch-1.8.1?(benjamin)
I just submitted a stack (under doronr@gmail.com), will take a few days to be processed though.
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.
(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?
(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.
(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?
How about ifdef STRIP_XPI ? And only set STRIP_XPI for xforms/lightning?
Attachment #206323 - Flags: branch-1.8.1?(benjamin)
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.
Attached patch STRIP_XPI patchSplinter Review
Attachment #210896 - Flags: review?(benjamin)
Comment on attachment 210896 [details] [diff] [review]
STRIP_XPI patch

This should be safe for the branches also.
Attachment #210896 - Flags: review?(benjamin)
Attachment #210896 - Flags: review+
Attachment #210896 - Flags: branch-1.8.1+
Attachment #210896 - Flags: approval1.8.0.2?
Comment on attachment 206323 [details] [diff] [review]
oops, missed the target

obseleting
Attachment #206323 - Attachment is obsolete: true
checked into 1.8.1
Whiteboard: fixed1.8.1
Mass reassign of open bugs for chase@mozilla.org to build@mozilla-org.bugs.
Assignee: chase → build
Status: REOPENED → NEW
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 on attachment 210896 [details] [diff] [review]
STRIP_XPI patch

global build/config changes scary for 1.8.0.x
Attachment #210896 - Flags: approval1.8.0.2? → approval1.8.0.2-
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.
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".
Attachment #212809 - Flags: approval1.8.0.2?
Attachment #212809 - Flags: review+
Assignee: build → doronr
Comment on attachment 212809 [details] [diff] [review]
1.8.0 branch version

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #212809 - Flags: approval1.8.0.2? → approval1.8.0.2+
thanks for approval, checked in.
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Whiteboard: fixed1.8.1 → fixed1.8.1, fixed1.8.0
Whiteboard: fixed1.8.1, fixed1.8.0 → fixed1.8.1, fixed1.8.0.2
moving keywords from whiteboard to keywrods
Whiteboard: fixed1.8.1, fixed1.8.0.2
Whiteboard: [nvn-dl]
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?
(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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.