The default bug view has changed. See this FAQ.

makexpi.pl only looks for *.so when stripping

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: jhp (no longer active), Assigned: Doron Rosenberg (IBM))

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
PowerPC
Mac OS X
fixed1.8.0.2, fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nvn-dl])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
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.
(Assignee)

Comment 2

12 years ago
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
(Assignee)

Comment 3

12 years ago
bsmedberg doron: gcc includes symbols by default
(Assignee)

Comment 4

11 years ago
Created attachment 205665 [details] [diff] [review]
patch

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

Comment 5

11 years ago
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.
(Assignee)

Updated

11 years ago
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-
(Assignee)

Comment 7

11 years ago
Created attachment 206323 [details] [diff] [review]
oops, missed the target

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?
(Assignee)

Comment 10

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

Comment 11

11 years ago
checked into trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

11 years ago
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-

Comment 14

11 years ago
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

11 years ago
I'm getting the same error as Comment #14 with a Firefox build on Win32/MingW/cygwin
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

11 years ago
(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

11 years ago
(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

11 years ago
I can also confirm failed builds using MSVC 7.1 on Win32. Same exact error as posted above.
(Assignee)

Comment 19

11 years ago
(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
(Assignee)

Comment 20

11 years ago
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

11 years ago
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

11 years ago
(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...

Comment 24

11 years ago
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

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

Comment 26

11 years ago
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)
(Assignee)

Comment 28

11 years ago
(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.
(Assignee)

Comment 29

11 years ago
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.
(Assignee)

Comment 30

11 years ago
suggested on irc:

what does |which find| return for you?

Comment 31

11 years ago
(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.
(Assignee)

Comment 33

11 years ago
(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

11 years ago
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

11 years ago
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)
(Assignee)

Comment 37

11 years ago
I just submitted a stack (under doronr@gmail.com), will take a few days to be processed though.

Comment 38

11 years ago
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.
(Assignee)

Comment 39

11 years ago
(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?
(Assignee)

Comment 40

11 years ago
(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.
(Assignee)

Comment 41

11 years ago
(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)

Comment 43

11 years ago
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.
(Assignee)

Comment 44

11 years ago
Created attachment 210896 [details] [diff] [review]
STRIP_XPI patch
(Assignee)

Updated

11 years ago
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?
(Assignee)

Comment 46

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

obseleting
Attachment #206323 - Attachment is obsolete: true
(Assignee)

Comment 47

11 years ago
checked into 1.8.1
Whiteboard: fixed1.8.1

Comment 48

11 years ago
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.
(Assignee)

Comment 52

11 years ago
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".
Attachment #212809 - Flags: approval1.8.0.2?
Attachment #212809 - Flags: review+
(Assignee)

Updated

11 years ago
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+
(Assignee)

Comment 54

11 years ago
thanks for approval, checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 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
Keywords: fixed1.8.0.2, fixed1.8.1
Whiteboard: fixed1.8.1, fixed1.8.0.2

Updated

11 years ago
Whiteboard: [nvn-dl]

Comment 56

11 years ago
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?
(Assignee)

Comment 57

11 years ago
(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.
You need to log in before you can comment on or make changes to this bug.