51465 - NSPR Linux opt target does not strip shared libraries

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, defect, P1)

Description

[Doug Turner (:dougt)]

The standalone nspr target with the right options do not seems to build it
properly (eg stripped).  Take a look: 

From the nightly tar: 
[dougt@z package]$ file libnspr4.so 
libnspr4.so: ELF 32-bit LSB shared object, Intel 80386, version 1, stripped 

From my local optimized tree: 
[dougt@z package]$ file libnspr4.so 
libnspr4.so: ELF 32-bit LSB shared object, Intel 80386, version 1, not stripped

Comment 1

[Wan-Teh Chang]

So the NSPR library in the nightly build is
built in a different way?

Comment 2

[Doug Turner (:dougt)]

no idea.  Leaf, cls, can you shed some light on this please?

Comment 3

[Daniel (Leaf) Nunes]

It isn't built differently, but part of the packaging process in the build
automation strips everything under dist/bin, which is why there's a difference.

Comment 4

[Jon Granrose]

well, we do build nspr before building mozilla and put it in a separate OBJDIR
(something I mean to change), but leaf is correct.  We strip everything under
dist/bin other than a few types of non-binary files as part of the packaging.
See mozilla/xpinstall/packager/Makefile.in.

We use the standard configure flags.  See /u/cltbld/logs/make_moz.linux22 for
the configure lines and environment variables.

Comment 5

[Judson Valeski]

nominating, this adds bloat to embedding clients.

Comment 6

[Wan-Teh Chang]

Reassigned to larryh.

Comment 7

[larryh (gone)]

A work-around for this bug already exists: The installer strips symbols. This 
seems like an OK solution.

Comment 8

[larryh (gone)]

Absent objections by 29-Sep-2000, NSPR will mark this closed WONTFIX.

Comment 9

[Doug Turner (:dougt)]

Larry, most customers of nspr do not use this installer.  We need to fix the
build.

Comment 10

[larryh (gone)]

No other products using NSPR need to have the optimized build further processed. 
I would think that the mozilla build process would do what it needs to do after 
we've built standard parts suitable for the rest of the universe.

I also have to ask: why is the provided installer not used?

Comment 11

[Judson Valeski]

embedding isn't far enough along to have an installer (it's not clear what an
embedding installer would even do). it would seem to me that NSPR should clean
up the bits as best as it can before people use them.

Comment 12

[Wan-Teh Chang]

Do you only want the optimized libraries to be stripped?
I assume that you want the symbols in the debug libraries,
right?

Do we lose anything if we strip a library?  Are all the
system libraries (e.g., libc.so, libpthread.so) stripped?

Comment 13

[larryh (gone)]

Attachment: proposed patch

Comment 14

[larryh (gone)]

See the proposed patch in the attachment.
The strip operation reduces NSPR's .so from 214K to 169K, about 20%

Wan-Teh, Please review.

Comment 15

[larryh (gone)]

Attachment: proposed patch, as "cvs diff -u" output

Comment 16

[larryh (gone)]

added attachment. patch is "cvs diff -u" output.

Comment 17

[Judson Valeski]

wan-teh, can we get a plus on this and get it in?

Comment 18

[larryh (gone)]

cls,
Please review my patch.
Is there anything that needs doing for autoconf?
/Larry.

Comment 19

[Wan-Teh Chang]

Judson, this bug is P3, nsbeta3.  I can't check it in
unless it gets the appropriate priority and status
according to the current policy.

Comment 20

[cls]

Attachment: changes to autoconf bits needed for strip

Comment 21

[Judson Valeski]

now it's P1. please proceed w/ checkin process.

Comment 22

[larryh (gone)]

Attachment: patch. Merged autoconf & other patch. file: configure not included

Comment 23

[larryh (gone)]

attached merged patch for Chris' autoconf patch and non-autoconf patches, fitted 
to NSPRPUB_CLIENT_BRANCH.

Review, please.

Comment 24

[cls]

It looks like a hunk fell out during the merge.  This was part of larry's
original patch and is needed for the strip to actually occur when using
--enable-nspr-autoconf.  Add this in and r=cls.

Index: rules.mk
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/config/rules.mk,v
retrieving revision 3.31
diff -u -r3.31 rules.mk
--- rules.mk    2000/08/20 23:52:43     3.31
+++ rules.mk    2000/09/22 17:53:01
@@ -315,6 +315,7 @@
        rm -f $@
 ifdef USE_AUTOCONF
        $(MKSHLIB) $(OBJS) $(EXTRA_LIBS) $(OS_LIBS)
+       $(STRIP) $@
 else
 ifeq ($(OS_ARCH)$(OS_RELEASE), AIX4.1)
        echo "#!" > $(OBJDIR)/lib$(LIBRARY_NAME)_syms

Comment 25

[Wan-Teh Chang]

cls: should we only strip the libs if BUILD_OPT is defined
(i.e., when building optimized)?

Comment 26

[cls]

Oops, that's right.  Hmmm...we're not setting BUILD_OPT if --enable-optimize is
given to configure.  I'll whip up a patch to fix that.

Comment 27

[lchiang]

per email with Jud, changing nsbeta3+ to nsbeta3- on all "embed" keyword bugs
since embedding changes will not be made in the mn6 branch. If you feel this bug
fix needs to go into mn6 branch, please list the reasons/user impact/risk and
nominate for rtm. Thanks.

Comment 28

[cls]

Attachment: Only call STRIP if BUILD_OPT is set. Includes all previous patches minus configure changes

Comment 29

[Judson Valeski]

so who's checking in the patches? they need to go into the trunk.

Comment 30

[larryh (gone)]

Checked in parts to the NSPR tip of tree.

Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.27; previous revision: 1.26
done
config/Linux.mk
Checking in config/Linux.mk;
/cvsroot/mozilla/nsprpub/config/Linux.mk,v  <--  Linux.mk
new revision: 3.22; previous revision: 3.21
done
config/UNIX.mk
Checking in config/UNIX.mk;
/cvsroot/mozilla/nsprpub/config/UNIX.mk,v  <--  UNIX.mk
new revision: 3.16; previous revision: 3.15
done
config/autoconf.mk.in
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/nsprpub/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 1.9; previous revision: 1.8
done
config/rules.mk
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.32; previous revision: 3.31
done

Comment 31

[Wan-Teh Chang]

This patch breaks the NSPR build on Windows
because STRIP is not defined.

Comment 32

[Judson Valeski]

then lets do it for unixes. agreed?

Comment 33

[Wan-Teh Chang]

Judson: yes.  This is easy to fix.  Just need to
move the $(STRIP) command inside the ifdef for Unixes.

Comment 34

[cls]

I'd prefer to just set STRIP=echo in WIN32.mk rather than have more ifdefs.

Brian Ryner has pointed out other problems with the patches...
1) configure.in is missing a AC_SUBST(STRIP)
2) configure was never checked in.  Afaik, it's not autogen'd like mozilla's is.
3) I added the strip option to the $(LIBRARY) target which doesn't cause
problems on my box but does on others.

And the changes never made it onto the NSPRPUB_CLIENT_BRANCH.

Comment 35

[cls]

Attachment: Added STRIP define to other <platform>.mks, removes strip from $(LIBRARY) target, includes configure changes

Comment 36

[cls]

Attachment: Contains sum of previous changes needed to be checked into the NSPRPUB_CLIENT_BRANCH branch

Comment 37

[Brian Ryner (not reading)]

r=bryner

Comment 38

[cls]

Changes from attach 15889 have been checked in.

Checking in nsprpub/configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.24; previous revision: 1.23
done
Checking in nsprpub/configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.28; previous revision: 1.27
done
Checking in nsprpub/config/BeOS.mk;
/cvsroot/mozilla/nsprpub/config/BeOS.mk,v  <--  BeOS.mk
new revision: 3.5; previous revision: 3.4
done
Checking in nsprpub/config/OS2.mk;
/cvsroot/mozilla/nsprpub/config/OS2.mk,v  <--  OS2.mk
new revision: 3.12; previous revision: 3.11
done
Checking in nsprpub/config/WIN32.mk;
/cvsroot/mozilla/nsprpub/config/WIN32.mk,v  <--  WIN32.mk
new revision: 3.13; previous revision: 3.12
done
Checking in nsprpub/config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.33; previous revision: 3.32
done
Checking in nsprpub/config/win16.mk;
/cvsroot/mozilla/nsprpub/config/win16.mk,v  <--  win16.mk
new revision: 3.7; previous revision: 3.6
done

Comment 39

[cls]

Changes from attach 15901 have been checked into the NSPRPUB_CLIENT_BRANCH :

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.17.2.5; previous revision: 1.17.2.4
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.18.2.5; previous revision: 1.18.2.4
done
Checking in config/BeOS.mk;
/cvsroot/mozilla/nsprpub/config/BeOS.mk,v  <--  BeOS.mk
new revision: 3.2.72.1; previous revision: 3.2
done
Checking in config/Linux.mk;
/cvsroot/mozilla/nsprpub/config/Linux.mk,v  <--  Linux.mk
new revision: 3.16.2.3; previous revision: 3.16.2.2
done
Checking in config/OS2.mk;
/cvsroot/mozilla/nsprpub/config/OS2.mk,v  <--  OS2.mk
new revision: 3.7.18.3; previous revision: 3.7.18.2
done
Checking in config/UNIX.mk;
/cvsroot/mozilla/nsprpub/config/UNIX.mk,v  <--  UNIX.mk
new revision: 3.13.72.1; previous revision: 3.13
done
Checking in config/WIN32.mk;
/cvsroot/mozilla/nsprpub/config/WIN32.mk,v  <--  WIN32.mk
new revision: 3.9.32.2; previous revision: 3.9.32.1
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/nsprpub/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 1.8.2.1; previous revision: 1.8
done
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.26.2.5; previous revision: 3.26.2.4
done
Checking in config/win16.mk;
/cvsroot/mozilla/nsprpub/config/win16.mk,v  <--  win16.mk
new revision: 3.4.72.1; previous revision: 3.4
done

Comment 40

[cls]

Marking fixed.

Comment 41

[Jon Granrose]

marking verified.