259945 - Mozilla should not use LD_LIBRARY_PATH on solaris

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Boying Lu]

On Solaris, a software should rely on LD_LIBRARY_PATH to locate its shared
libraries, instead it should use $ORIGIN to locate shared libraries.  I think it
should be same on Linux

Comment 1

[Boying Lu]

Attachment: patch v0

before building mozilla for solaris, we should also set
LDFLAGS=-R'$ORIGIN:$ORIGIN/..:$ORIGIN/plugins' and set CXXFLAGS+='-norunpath'

Comment 2

[Boying Lu]

Comment on attachment 159159 [details] [diff] [review]
patch v0

Can you review the patch?
Thanks

Comment 3

[Julien Pierre]

I disagree that the "$ORIGIN/..:$ORIGIN/plugins" directories needs to be in the
rpath. Plugins is a mozilla directory, which has nothing to do with NSPR .
$ORIGIN/.. seems to be installation specific. These don't help resolve any
dependencies of the NSPR shared libraries..

As far as just adding $ORIGIN to the rpath, we should do that for libplc4.so and
libplds4.so, which both depend on libnspr4.so .

Comment 4

[Wan-Teh Chang]

Comment on attachment 159159 [details] [diff] [review]
patch v0

Hi, I can't take this patch in its current form
because 1) $$ORIGIN/plugins is specific to the
Mozilla client (NSPR and NSS are used by other
products, too), and 2) this patch conflicts with
the -R settings in our other makefiles.

If you have product requirement to NOT use
LD_LIBRARY_PATH with Mozilla, we can consider using
the MOZILLA_CLIENT make variable to customize
our build for you.  I want to avoid that if possible.

By the way, $$ORIGIN/plugins is only needed for
libnspr4.so because Mozilla uses the PR_LoadLibrary
functions in that library to load the plugins.

Comment 5

[Wan-Teh Chang]

Julien, the interaction between dlopen and -R is
very interesting.

The mozilla executable uses PR_LoadLibrary to load
a plugin.  PR_LoadLibrary is a function defined in
libnspr4.so.  PR_LoadLibrary calls dlopen.  The
non-obvious fact is that dlopen honors the -R
setting on libnspr4.so (where it is located), not
the -R setting on the mozilla executable.

This is why the bug submitter wanted to add
-R $ORIGIN/plugins to libnspr4.so.

Comment 6

[Julien Pierre]

Wan-Teh,

Thanks for the explanation, I wasn't aware of this issue.

I think, if at all possible, for the mozilla client, we should continue to rely
on LD_LIBRARY_PATH for this case.

The only case I am aware of under which this will not work is if you have an
suid application, ie. an application which doesn't start as root, but can change
user. LD_LIBRARY_PATH is ignored in that situation for security reasons.

However, in that case, $ORIGIN also will not be resolved, for the same security
reasons.

The only true fix for suid applications is a hardcoded rpath. I don't think
mozilla should ever need to be an suid application, however, so either
LD_LIBRARY_PATH or $ORIGIN would be fine.

If we really want to change the rpath for this case, then indeed we would need
to use a MOZILLA_CLIENT macro.

Also, we have found out that the linker only supports a single -R . Currently,
many Makefiles, in NSPR and NSS, use the += syntax for setting rpath. The
makefile system will need to be slightly reworked if the rpath is being set from
different files. Ie. have one RPATHOPT macro that resolves '-R $RPATH', and then
we can use += syntax on the RPATH variable .

Comment 7

[Boying Lu]

Hi, All,
  Thanks for your feedback. My point is Mozilla should not relay on
LD_LIBRARY_PATH to locate shared libraries. This is a requirment for software on
solaris.
  if $ORIGIN/plugins is used only for libnspr4.so, I can modified the patch. An
other point is we can hardcode /usr/lib into rpath. The final rpath may be
looked like this:
for shared libraries under dist/bin:
   /usr/lib:$ORIGIN
for shared libraries under dist/bin/components:
   /usr/lib:$ORIGIN/..
and we should set LD_LIBRARY_PATH to "" in run-mozilla.sh to prevent user has a
long path list in LD_LIBRARY_PATH, which will hurt mozilla startup performance.

Comment 8

[Boying Lu]

Attachment: an configure option --enable-origin is added

I propose a better solution, I add an --enable-origin to add
"$ORIGIN:$ORIGIN/.." to runpath, which will cause mozilla will not depend on
LD_LIBRARY_PATH to locate shared libraries. I also set LD_LIBRARY_PATH to ""
when mozilla is running in non-debug mode on solaris. This will speed up
mozilla startup performance.

In debug mode, Mozilla root directory will be added to LD_LIBRARY_PATH, which
is convenient for developers.

In sumarray, when build a release version for solaris, we should use
--enable-origin.

Comment 9

[Boying Lu]

BTW: $ORIGIN/plugins is not needed, because plugin directory is "hard coded" in
xpti.dat file and mozilla use it to load all plugin libraries.

Comment 10

[Boying Lu]

Comment on attachment 160131 [details] [diff] [review]
an configure option --enable-origin is added

Can you give r? thanks

Comment 11

[Wan-Teh Chang]

Comment on attachment 160131 [details] [diff] [review]
an configure option --enable-origin is added

1. In comment 9 you said $ORIGIN/plugins is not needed.
But this patch still contains $ORIGIN/plugins.

2. You should modify configure.in instead of configure,
and then run "autoconf" to generate configure.	The
patch should not include the changes to configure so
that it is easier to review.

3. In nsprpub/configure, you have:

>+# Check whether --enable-origin or --disable-origin was given.
>+if test "${enable_origin+set}" = set; then   
>+  if test $OS_ARCH = "SunOS"; then
>+    LDFLAGS="-R'\$\$ORIGIN:\$\$ORIGIN/plugins' $LDFLAGS"
>+    CXXFLAGS="-norunpath $CXXFLAGS"
>+  fi
>+fi

What does the C++ compiler option -norunpath mean?
Do you need a similar option for the C compiler?

As Julien pointed out, only one -R option can be
specified.  So we need a better solution because
our makefiles already use the -R option.  (For
example, see nsprpub/lib/ds/Makefile.in.)

4. In build/unix/run-mozilla.sh, you have:

>+if [ `uname -s ` = "SunOS"]; then
>+  if [ "$moz_debug" -ne 1 ]; then
>+    LD_LIBRARY_PATH=""
>+  fi
>+fi

In the latest gcc releases, libgcc is a shared
library.  gcc users usually adds the directory
where libgcc.so resides to their LD_LIBRARY_PATH.
So if you set LD_LIBRARY_PATH to an empty string,
it will break a Mozilla build compiled with the
latest gcc releases.

I also don't understand why you only want to
unset LD_LIBRARY_PATH in optimized builds.

Comment 12

[Boying Lu]

Yes. $ORIGIN/plugins should not be put in the patch. I will make another patch
soon. Thanks 

Comment 13

[Boying Lu]

 "-norunpath" means that disable building the default path for shared libraries
into the executable. This will improve the speed of locating shared libraries.

Comment 14

[Boying Lu]

Attachment: a new proposal

The new patch made the following changes:
1. remove $ORIGIN/plugins from runpath, because it is not required

2. the patch will take no effect if one of the following conditions is not
meet:
   a. --enable-origin is not specified (or --disable-origin is specified)
   b. mozilla is build on solaris platform and Sun Compiler is used
   c. mozilla is build for Linux and GCC is used 

3. left direcotry/c-sdk/configure.in unchanged, because the shared libraries
under this directory only depend on "standard" shared libraries, so no runpath
is needed.

4. I still think we should set LD_LIBRARY_PATH to empty in run-mozilla.sh. I
use  gcc 3.2.2 to build mozilla on linux and mozilla still can be lanuched with
 empty LD_LIBRARY_PATH. 

The purpose to set LD_LIBRARY_PATH to empty is to speed up mozilla startup:
On Linux, runpath takes precedence over LD_LIBRARY_PATH, I.e. ld.so.1 will try
runpath first and then LD_LIBRARY_PATH.

But on solaris, it is reversed, ld.so.1 will try LD_LIBRARY_PATH first. So on
solaris, if a user has a long directory list in LD_LIBRARY_PATH, ld.so.1 will
waste times to search directories in LD_LIBRARY_PATH.

If you agree, I will made another patch for run-mozilla.sh

I have tested the patch on both solaris (using Sun compiler)and Linux (using
GCC). it works.

It is required that softwares to be integivated with solaris should not rely on
LD_LIBRARY_PATH to locate its shared libraries and it is not a recommend way to
  use LD_LIBRARY_PATH to locate shared libraries on Linux neither.

My suggestion is: When make a release build, "--enable-origin" should be
enabled, for debug build, it is not required.

Comment 15

[Boying Lu]

Comment on attachment 160907 [details] [diff] [review]
a new proposal

Can you give r now? Thanks

Comment 16

[Wan-Teh Chang]

Comment on attachment 160907 [details] [diff] [review]
a new proposal

1. mozilla/configure.in

>+MOZ_ARG_DISABLE_BOOL(origin,
>+[  --enable-origin         Use \$ORIGIN to locate shared libraries instead of \
>+LD_LIBRARY_PATH (solaris only)],
>+    ENABLE_ORIGIN=1,
>+    ENABLE_ORIGIN=0)

"ENABLE_ORIGIN=0" should be replaced by "ENABLE_ORIGIN=".
This may require using test -n or test -z with
$ENABLE_ORIGIN.

>+if test "$ENABLE_ORIGIN" ; then
>+ if test "$OS_ARCH" = "SunOS" && test "$GCC" != "yes"; then
>+    LDFLAGS="-R'\$\$ORIGIN:\$\$ORIGIN/..' $LDFLAGS"
>+    CXXFLAGS="-norunpath $CXXFLAGS"
>+ else 
>+    if test "$OS_ARCH" = "Linux" && test "$GCC" = "yes"; then
>+      CFLAGS="-Wl,-rpath,'\$\$ORIGIN:\$\$ORIGIN/..' $CFLAGS"
>+      CXXFLAGS="-Wl,-rpath,'\$\$ORIGIN:\$\$ORIGIN/..' $CXXFLAGS"
>+    fi
>+ fi
>+fi

You should also handle the Solaris/gcc case.

The help message you added above says this is "solaris only",
but you also use $ORIGIN on Linux.

2. mozilla/nsprpub/configure.in

>+dnl ========================================================
>+dnl =
>+dnl = --enable-origin
>+dnl = 
>+dnl = Set $ORIGIN:$ORIGIN/.. into shared libraries runpath
>+dnl = 
>+dnl ========================================================
>+AC_ARG_ENABLE(origin,
>+    [  --enable-origin         Use \$ORIGIN to locate shared libraries instead of LD_LIBRARY_PATH (solaris only) ], 
>+     [ if test "$enableval" = "yes"; then
>+	    ENABLE_ORIGIN=1
>+      fi ])
>+
>+if test "$ENABLE_ORIGIN" ; then
>+  if test "$OS_ARCH" = "SunOS" && test "$GCC" != "yes"; then
>+    LDFLAGS="-R'\$\$ORIGIN' $LDFLAGS"
>+    CXXFLAGS="-norunpath $CXXFLAGS"
>+  else 
>+    if test "$OS_ARCH" = "Linux" && test "$GCC" = "yes"; then
>+      DSO_LDOPTS="-Wl,-rpath,'\$\$ORIGIN' $DSO_LDOPTS"
>+    fi
>+  fi
>+fi

For NSPR, the comment conflicts with the code.
Should $ORIGIN/.. be set in the shared libraries'
run path?

Comment 17

[Wan-Teh Chang]

Comment on attachment 160907 [details] [diff] [review]
a new proposal

If you only need to add $ORIGIN to NSPR, we should
always add it and get rid of the --enable-origin
configure option.

Comment 18

[Wan-Teh Chang]

Regarding LD_LIBRARY_PATH and libgcc_s.so, please
see bug 200179, bug 200537, and bug 203931.

Comment 19

[Boying Lu]

$ORIGIN/.. is not needed by the shared libraries under nspr/, because there are
only three shared libraries: 
nspr/lib/ds/libplds4.so
nspr/lib/libc/src/libplc4.so
nspr/pr/src/libnspr4.so

The first two only depends on libnspr4.so and some "standard" shared libraries
and these three libraries are put in the same directory, so '$ORIGIN/..' is not
needed.

I think '--enable-origin' is needed, because:
1. $ORIGIN is not only used by shared libraries under nspr/ but also other
shared libraries, which depends on other mozilla shared libraries.  
2. User may choice not using this feature.

Comment 20

[Boying Lu]

I don't know why we should use "ENABLE_ORIGIN=" instead of "ENABLE_ORIGIN=0"? 
I found that "-enable-xft" option set MOZ_ENABLE_XFT=1 instead of
"MOZ_ENABLE_XFT=".  Could you tell me the reason?  Thanks

Comment 21

[Boying Lu]

I will add "GCC" check for solaris

Comment 22

[Boying Lu]

For the issue related to libgcc_s.so.1, I think it has nothing to do with this
patch, The mozilla binrary codes  DO NOT depends on any gcc related libraries.
I will further investigate this issue.

Comment 23

[Boying Lu]

Attachment: another proposal

Compared with previous one I made following modifications:
1. add the support for GCC on solaris platform

2. add '$ORIGIN' support in directory/c-sdk/configure.in for solaris platform,
because libprldap50.so depends on libnspr4.so,libplc4.so,libplds4.so and
libldap4.so, but for Linux it doesn't

3. Using DSO_LDOPTS instead of CXXFLAGS and CFLAGS in nsprpub/configure.in and
directory/c-sdk/configure.in, because it is used to build shared libraries on
both  platform.

4. Use "test -n $ENABLE_ORIGIN" instead of "test $ENABLE_ORIGIN" to void
portable issue.

Comment 24

[Boying Lu]

Comment on attachment 161542 [details] [diff] [review]
another proposal

can you give r now? Thanks

Comment 25

[Boying Lu]

I still think we should set LD_LIBRARY_PATH to empty ( and use --enable-origin)
in release build. So mozilla will not rely on LD_LIBRARY_PATH to locate shared
libraries and improve startup performance.

Comment 26

[Boying Lu]

Attachment: only set $ORIGIN:$ORIGIN/.. for solaris platform

Comment 27

[Wan-Teh Chang]

Comment on attachment 197151 [details] [diff] [review]
only set $ORIGIN:$ORIGIN/.. for solaris platform

You attached the wrong patch.:-)

Comment 28

[Boying Lu]

Comment on attachment 197151 [details] [diff] [review]
only set $ORIGIN:$ORIGIN/.. for solaris platform

I'm sorry :(

Comment 29

[Boying Lu]

Attachment: patch

Don't rely on LD_LIBRARY_PATH to locate shared library on solaris platform.

Comment 30

[Wan-Teh Chang]

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

The NSPR change is no longer necessary.  We added
-R '$$ORIGIN' to NSPR (specifically libplc4.so and
libplds4.so. libnspr4.so doesn't need -R '$$ORIGIN)
in bug 244095.

I suggest that you ask someone who knows shell and
make programming to review the remaining changes.
I can recommend cls (Chris Seawood) or Brian Smedberg.

I have some comments and questions below.

In configure.in, you have:

>        DSO_LDOPTS='-G -Qoption ld -z,muldefs -h $@'
>+       DSO_LDOPTS="$DSO_LDOPTS -R '\$\$ORIGIN:\$\$ORIGIN/..'"

Are you sure you need the backslashes inside the single
quotes?

It would be good to add a comment to explain what
$$ORIGIN/.. is for.

In build/unix/run-mozilla.sh, you have

> ##
> ## Set LD_LIBRARY_PATH
>-LD_LIBRARY_PATH=${MOZ_DIST_BIN}:${MOZ_DIST_BIN}/plugins:${MRE_HOME}${LD_LIBRARY_PATH+":$LD_LIBRARY_PATH"}
>+if [ -h "$MOZ_PROGRAM" -o `uname -s` != "SunOS" ]
>+then
>+  LD_LIBRARY_PATH=${MOZ_DIST_BIN}:${MOZ_DIST_BIN}/plugins:${MRE_HOME}${LD_LIBRARY_PATH+":$LD_LIBRARY_PATH"}
>+fi

Please add a comment to explain why we don't need to set
LD_LIBRARY_PATH on SunOS (Solaris).

What is the -h "$MOZ_PROGRAM" test for?

Why don't you need ${MRE_HOME}?

Comment 31

[Wan-Teh Chang]

Sorry, that should be Benjamin Smedberg <[e-mail]>.
He knows Mozilla's build system very well.

Comment 32

[Benjamin Smedberg]

Just so I can catch up here: what *is* an origin? is it like executable_path on
mac, but is relative to the shared library location? If we don't need to set
LD_LIBRARY_PATH, why don't we just skip the shell script and make "firefox" the
executable binary, instead of going through the firefox-bin mumbo-jumbo?

Comment 33

[Boying Lu]

(In reply to comment #30)
> (From update of attachment 197379 [details] [diff] [review] [edit])
> The NSPR change is no longer necessary.  We added
> -R '$$ORIGIN' to NSPR (specifically libplc4.so and
> libplds4.so. libnspr4.so doesn't need -R '$$ORIGIN)
> in bug 244095.
> 
> I suggest that you ask someone who knows shell and
> make programming to review the remaining changes.
> I can recommend cls (Chris Seawood) or Brian Smedberg.
> 
> I have some comments and questions below.
> 
> In configure.in, you have:
> 
> >        DSO_LDOPTS='-G -Qoption ld -z,muldefs -h $@'
> >+       DSO_LDOPTS="$DSO_LDOPTS -R '\$\$ORIGIN:\$\$ORIGIN/..'"
> 
> Are you sure you need the backslashes inside the single
> quotes?
> 
> It would be good to add a comment to explain what
> $$ORIGIN/.. is for.
> 
> In build/unix/run-mozilla.sh, you have
> 
> > ##
> > ## Set LD_LIBRARY_PATH
>
>-LD_LIBRARY_PATH=${MOZ_DIST_BIN}:${MOZ_DIST_BIN}/plugins:${MRE_HOME}${LD_LIBRARY_PATH+":$LD_LIBRARY_PATH"}
> >+if [ -h "$MOZ_PROGRAM" -o `uname -s` != "SunOS" ]
> >+then
> >+ 
LD_LIBRARY_PATH=${MOZ_DIST_BIN}:${MOZ_DIST_BIN}/plugins:${MRE_HOME}${LD_LIBRARY_PATH+":$LD_LIBRARY_PATH"}
> >+fi
> 
> Please add a comment to explain why we don't need to set
> LD_LIBRARY_PATH on SunOS (Solaris).
> 
> What is the -h "$MOZ_PROGRAM" test for?
> 
> Why don't you need ${MRE_HOME}?
> 

The '-h "$MOZ_PROGRAM"' is to test if seamonkey-bin is a symobl link or not
The reason to test this is that $ORIGIN doesn't work on symbol links.
I.e. we can't invoke mozilla under dist/bin/ without setting dist/bin into
LD_LIBRARY_PATH 

For end user, it is not an issue because he will never use Mozilla like this.
But for a developer, he often need to invoke mozilla under dist/bin/, he had to
set dist/bin into LD_LIBRARY_PATH manually.

By testing '-h "$MOZ_PROGRAM"', we can know if the mozilla is invoked under
dist/bin or not and set dist/bin into LD_LIBRARY_PATH automatically in that case.

 

Comment 34

[Boying Lu]

(In reply to comment #32)
> Just so I can catch up here: what *is* an origin? is it like executable_path on
> mac, but is relative to the shared library location? If we don't need to set
> LD_LIBRARY_PATH, why don't we just skip the shell script and make "firefox" the
> executable binary, instead of going through the firefox-bin mumbo-jumbo?

The $ORIGIN is a keyword for ld.so.1. It means the directory of the current
shared library. If it is set into a shared library's runpath, ld.so.1 can use it
to locate shared libraries which the shared library depends on. Just like we use
relative path to locate a file in file system.

On solaris, it is an requirement not to rely on LD_LIBRARY_PATH to locate shared
libraries. So we need to use $ORIGIN to locate shared libraries. That is what
the patch does.

I think we can't run firefox-bin directory, because "firefox" do some other
things except setting LD_LIBRARY_PATH. I think we can simply "firefox" script
rather than removing it.

Comment 35

[Boying Lu]

(In reply to comment #30)
> (From update of attachment 197379 [details] [diff] [review] [edit])
> The NSPR change is no longer necessary.  We added
> -R '$$ORIGIN' to NSPR (specifically libplc4.so and
> libplds4.so. libnspr4.so doesn't need -R '$$ORIGIN)
> in bug 244095.
> 
> I suggest that you ask someone who knows shell and
> make programming to review the remaining changes.
> I can recommend cls (Chris Seawood) or Brian Smedberg.
> 
> I have some comments and questions below.
> 
> In configure.in, you have:
> 
> >        DSO_LDOPTS='-G -Qoption ld -z,muldefs -h $@'
> >+       DSO_LDOPTS="$DSO_LDOPTS -R '\$\$ORIGIN:\$\$ORIGIN/..'"
> 
> Are you sure you need the backslashes inside the single
> quotes?
> 
The backslashes are needed or $$ will expand to pid.

> It would be good to add a comment to explain what
> $$ORIGIN/.. is for.
$$ORIGIN/.. is set for shared libraries under components/ to locate shared
libraries in upper level. I will add comments in my patch
> 
> In build/unix/run-mozilla.sh, you have
> 
> > ##
> > ## Set LD_LIBRARY_PATH
>
>-LD_LIBRARY_PATH=${MOZ_DIST_BIN}:${MOZ_DIST_BIN}/plugins:${MRE_HOME}${LD_LIBRARY_PATH+":$LD_LIBRARY_PATH"}
> >+if [ -h "$MOZ_PROGRAM" -o `uname -s` != "SunOS" ]
> >+then
> >+ 
LD_LIBRARY_PATH=${MOZ_DIST_BIN}:${MOZ_DIST_BIN}/plugins:${MRE_HOME}${LD_LIBRARY_PATH+":$LD_LIBRARY_PATH"}
> >+fi
> 
> Please add a comment to explain why we don't need to set
> LD_LIBRARY_PATH on SunOS (Solaris).
> 
> What is the -h "$MOZ_PROGRAM" test for?
> 
> Why don't you need ${MRE_HOME}?
> 

Comment 36

[Boying Lu]

Attachment: remove $ORIGIN in nspr/configure.in

Remove the $ORIGIN in nspr/configure.in

Comment 37

[Boying Lu]

Attachment: add some comments and remove $ORIGIN from nspr/configure.in

Comment 38

[Wan-Teh Chang]

Comment on attachment 197531 [details] [diff] [review]
add some comments and remove $ORIGIN from nspr/configure.in

In configure.in, we have:

>-       CFLAGS="$CFLAGS -xstrconst -xbuiltin=%all"
>-       CXXFLAGS="$CXXFLAGS -xbuiltin=%all -features=tmplife"
>+       # $ORIGIN/.. is for shared libraries under components/ to locate shared
>+       # libraries one level upper (e.g. libnspr4.so)
>+       CFLAGS="$CFLAGS -xstrconst -xbuiltin=%all -R '\$\$ORIGIN:\$\$ORIGIN/..'"
>+       CXXFLAGS="$CXXFLAGS -xbuiltin=%all -features=tmplife -norunpath -R '\$\$ORIGIN:\$\$ORIGIN/..'"

It seems wrong to add a linker option (-R) to CFLAGS
and CXXFLAGS, which are used when compiling every C
or C++ file.  I think the -R linker option should be
added to LDFLAGS and DSO_LDOPTS instead.

In the comment, "upper" should be "up".

In build/unix/run-mozilla.sh, we have:

> ##
> ## Set LD_LIBRARY_PATH
>-LD_LIBRARY_PATH=${MOZ_DIST_BIN}:${MOZ_DIST_BIN}/plugins:${MRE_HOME}${LD_LIBRARY_PATH+":$LD_LIBRARY_PATH"}
>+## -h is testing if $MOZ_PROGRAM is a symbol link, in this case we need to set 
>+## ${MOZ_DIST_BIN} into LD_LIBRARY_PATH because $ORIGIN doesn't work in symbol link
>+if [ -h "$MOZ_PROGRAM" -o `uname -s` != "SunOS" ]
>+then
>+  LD_LIBRARY_PATH=${MOZ_DIST_BIN}:${MOZ_DIST_BIN}/plugins:${MRE_HOME}${LD_LIBRARY_PATH+":$LD_LIBRARY_PATH"}
>+fi

The indentation level in that part of the file is
a tab, so you should also indent the
LD_LIBRARY_PATH=... line the same way (with a tab).

In the comment, "symbol link" should be "symbolic link",
"work in symbol link" should be "work on a symbolic link"
or "work for a symbolic link", and
"set ${MOZ_DIST_BIN} into LD_LIBRARY_PATH"
should be simply "set LD_LIBRARY_PATH".

Comment 39

[Boying Lu]

Attachment: a revision

Made some changes according to Wan-Teh's comments. Thanks Wan-Teh.

Comment 40

[Wan-Teh Chang]

Comment on attachment 197799 [details] [diff] [review]
a revision 

Brian, I still have questions.	In mozilla/configure.in,
you have:

>-       CXXFLAGS="$CXXFLAGS -xbuiltin=%all -features=tmplife"
>-       LDFLAGS="-xildoff -zlazyload -zcombreloc $LDFLAGS"
>+       CXXFLAGS="$CXXFLAGS -xbuiltin=%all -features=tmplife -norunpath"
>+       # $ORIGIN/.. is for shared libraries under components/ to locate shared
>+       # libraries one level up (e.g. libnspr4.so)
>+       LDFLAGS="-xildoff -zlazyload -zcombreloc $LDFLAGS -R '\$\$ORIGIN:\$\$ORIGIN/..'"

Should -norunpath be moved to LDFLAGS?

Why don't you add -R '\$\$ORIGIN:\$\$ORIGIN/..' to
DSO_LDOPTS?

In mozilla/build/unix/run-mozilla.sh, since there are
some restrictions on the use of $ORIGIN in a "secure process"
(see the Security section at the bottom of this page
http://docs.sun.com/app/docs/doc/817-1984/6mhm7pl33?a=view),
should we also set LD_LIBRARY_PATH when we detect we are a
"secure process"?  (I don't really know the definition of a
secure process.)

Comment 41

[Wan-Teh Chang]

Comment on attachment 197799 [details] [diff] [review]
a revision 

Brian, I found that $(LDFLAGS) is used in the makefile rules
for building programs and shared libraries.  Is this why you
don't need to add -R '\$\$ORIGIN:\$\$ORIGIN/..' to DSO_LDOPTS?

Comment 42

[Boying Lu]

(In reply to comment #41)
> (From update of attachment 197799 [details] [diff] [review] [edit])
> Brian, I found that $(LDFLAGS) is used in the makefile rules
> for building programs and shared libraries.  Is this why you
> don't need to add -R '\$\$ORIGIN:\$\$ORIGIN/..' to DSO_LDOPTS?
> 
Because I want seamonkey-bin also use $ORIGIN to locate shared libraries, I
can't use DSO_LDOPTS.

Comment 43

[Wan-Teh Chang]

Comment on attachment 197799 [details] [diff] [review]
a revision 

OK.  Then the only issue with this patch is the
-norunpath flag.  It should be either removed or
moved to LDFLAGS.

Comment 44

[Wan-Teh Chang]

Comment on attachment 197799 [details] [diff] [review]
a revision 

r=wtc.	I found that -norunpath is a C++ compiler
option.

Please ask someone else to review this patch, too.

Comment 45

[Wan-Teh Chang]

Comment on attachment 197799 [details] [diff] [review]
a revision 

>+## -h is testing if $MOZ_PROGRAM is a symbolic link, in this case we need to set 
>+## LD_LIBRARY_PATH because $ORIGIN doesn't work on symbolic link
>+if [ -h "$MOZ_PROGRAM" -o `uname -s` != "SunOS" ]
>+then

I suggest reversing `uname -s` != "SunOS" and -h "$MOZ_PROGRAM",
and write the comment like this:
 ## On Solaris we use $ORIGIN instead of LD_LIBRARY_PATH unless
 ## $MOZ_PROGRAM is a symbolic link, in this case we need to set
 ## LD_LIBRARY_PATH because $ORIGIN doesn't work on a symbolic link.

Comment 46

[Boying Lu]

Attachment: revision [Checkin: See comment 49]

Comment 47

[[e-mail]]

Comment on attachment 197969 [details] [diff] [review]
revision
[Checkin: See comment 49]

Don't forget to ask bsmedberg/chase for review.

Comment 48

[Boying Lu]

Can anyone help me to checkin this patch? Thanks 

Comment 49

[Wan-Teh Chang]

Attachment: Patch as checked in

I checked in the mozilla/configure.in and mozilla/build/unix/run-mozilla.sh
changes on the Mozilla trunk (1.9a).  I found that
the mozilla/directory/c-sdk/configure.in change is not
correct for the GCC case.  I will attach a new patch.

Comment 50

[Wan-Teh Chang]

Attachment: Patch for mozilla/directory/c-sdk/configure.in

Brian, could you review and test this patch on Solaris?

The reason the original change is wrong for the GCC case
is that MKSHLIB is defined as '$(LD) $(DSO_LDOPTS) -o $@'
for Solaris in mozilla/directory/c-sdk/configure.in, so
it is not necessary to use -Wl to pass linker flags in
DSO_LDOPTS.

Comment 51

[Boying Lu]

I've verfied that the patch works on solaris when building with Sun's compiler.
But there will be a problem when building with GCC. 

The root cause is that some Gnome's *.pc for solaris uses Sun compiler's  
specific options, which is not accepted by GCC.

I'm working with Sun Gnome team to resolve this issue. So please hold this patch
and I will give a new patch if this issue is resolved.

Comment 52

[Boying Lu]

Attachment: a new patch

The patch works on solaris

Comment 53

[Boying Lu]

Attachment: patch

I changed '-h "$MOZ_PROGRAM"' to '-h "libmozjs.so"' because firefox-bin and thunderbird-bin is not a symbol link under dist/bin.

Comment 54

[Wan-Teh Chang]

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

Have you tested this patch under GCC?
Does GCC also accept the -R option?

In build/unix/run-mozilla.sh,

> ## On Solaris we use $ORIGIN instead of LD_LIBRARY_PATH unless
> ## $MOZ_PROGRAM is a symbolic link, in this case we need to set
> ## LD_LIBRARY_PATH because $ORIGIN doesn't work on a symbolic link.
>-if [ `uname -s` != "SunOS" -o -h "$MOZ_PROGRAM" ]
>+if [ `uname -s` != "SunOS" -o -h "libmozjs.so" ]

The comment needs to be updated to match the code.
Perhaps it is more robust to test both:
    -h "$MOZ_PROGRAM" -o -h "libmozjs.so"

Comment 55

[Boying Lu]

Attachment: patch

update the comments in run-mozilla.sh
I don't think we should test both $MOZ_PROGRAM and libmozjs.so. Because dist/bin/libmozjs.so is a symbol link under seamonkey, firfox and thunderbird. 
There is no need to test '-h $MOZ_PROGRAM'.

Comment 56

[Wan-Teh Chang]

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

The comment should explain why you are testing
libmozjs.so and not the other shared libraries.
Is there something special about libmozjs.so?
Or is it just a representative shared library
(in other words, if libmozjs.so is a symlink,
then all the other shared libraries are also
symlinks)?

Comment 57

[Wan-Teh Chang]

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

I'd like to elaborate on my previous comment.

In the previous patch, the comment no longer
describes the new code.

In this patch, the comment describes the new
code, so it is an improvement over the previous
patch.  But the comment merely repeats the code
and does not explain or add any insight on why
$ORIGIN doesn't work if libmozjs.so is a symlink,
and among the dozens of shared libraries why you
only test if libmozjs.so is a symlink.
So it doesn't help the future maintainer of this
code.

Comment 58

[Boying Lu]

Attachment: patch

I made following changes in this patch:
1. re-write the comments in run-mozilla.sh
2. add the test for $MOZ_PROGRAM. 
   For reasons, please see the comments

Comment 59

[Wan-Teh Chang]

Attachment: New patch v6

Brian,  I edited the comment to make it more concise.
I also wrapped a long line.

Please review and test this patch.

I hope you have a better way to detect the case that
run-mozilla.sh is not invoked by the firefox, thunderbird,
or seamonkey script.  It is not good to hardcode these
three applications, because there will be other new Mozilla
client applications that need to be handled the same way.

Comment 60

[Boying Lu]

Attachment: a new proposal [Checkin: See comment 61]

I find a better way to detect the case and remove the hardcode part and related comments

Comment 61

[Wan-Teh Chang]

Comment on attachment 202454 [details] [diff] [review]
a new proposal
[Checkin: See comment 61]

I checked in the configure.in and run-mozilla.sh changes
in this patch on the Mozilla trunk (1.9a).

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1551; previous revision: 1.1550
done
Checking in build/unix/run-mozilla.sh;
/cvsroot/mozilla/build/unix/run-mozilla.sh,v  <--  run-mozilla.sh
new revision: 1.52; previous revision: 1.51
done

Comment 62

[Wayne Mery (:wsmwk)]

Comment on attachment 159159 [details] [diff] [review]
patch v0

cleared review? from obsolete patch

Comment 63

[Boying Lu]

Attachment: use libgkgfx.so instead of libmozjs.so [Checkin: Comment 68]

Comment 64

[Boying Lu]

libmozjs.so isn't a symbolic anymore in Firefox 3.1

Comment 65

[Wan-Teh Chang]

Comment on attachment 347921 [details] [diff] [review]
use libgkgfx.so instead of libmozjs.so
[Checkin: Comment 68]

r=wtc.

Comment 66

[Serge Gautherie (:sgautherie)]

What about the c-sdk part, which was not checked in in comment 61 ?

Is this bug actually SeaMonkey or should it be updated/moved to Core ?

Comment 67

[Boying Lu]

(In reply to comment #66)
> What about the c-sdk part, which was not checked in in comment 61 ?
>
That part shouldn't be checked in, user should use -with-rpath instead
 
> Is this bug actually SeaMonkey or should it be updated/moved to Core ?
The patch effects Firefox,Thunderbird and Seamonkey and should be changed to core

Comment 68

[Serge Gautherie (:sgautherie)]

Comment on attachment 347921 [details] [diff] [review]
use libgkgfx.so instead of libmozjs.so
[Checkin: Comment 68]

http://hg.mozilla.org/mozilla-central/rev/cfb281c007d0

Comment 69

[Ginn Chen]

libgkgfx.so is not as good as libxul.so

If I build Firefox with enable-libxul, I couldn't run Firefox from dist/bin because libgkgfx.so doesn't exist.

Comment 70

[Benjamin Smedberg]

in libxul builds, libgkgfx is a static library which is linked into libxul

Comment 71

[Boying Lu]

Attachment: use libxul.so instead of libgkgfx.so [Checkin: Comment 73 & 74]

Comment 72

[Wan-Teh Chang]

Comment on attachment 350260 [details] [diff] [review]
use libxul.so instead of libgkgfx.so
[Checkin: Comment 73 & 74]

r=wtc.

Comment 73

[Serge Gautherie (:sgautherie)]

Comment on attachment 350260 [details] [diff] [review]
use libxul.so instead of libgkgfx.so
[Checkin: Comment 73 & 74]

http://hg.mozilla.org/mozilla-central/rev/125f4a227cce

Comment 74

[Serge Gautherie (:sgautherie)]

Comment on attachment 350260 [details] [diff] [review]
use libxul.so instead of libgkgfx.so
[Checkin: Comment 73 & 74]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7d015d662ca7

Comment 75

[Ginn Chen]

I just find out libxul.so is still not good for Thunderbird.
spin off bug 472269.