325682 - rpath needs to be for some Linux builds

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Neil Williams]

On Linux there is a situation similar to 
https://bugzilla.mozilla.org/show_bug.cgi?id=245518 where we need to build with rpath set. This is for Sun packaging only so the fix should be conditional on the BUILD_SUN_PKG variable.

Comment 1

[Neil Williams]

Attachment: proposed fix

I tested this fix by building with patch applied for 32- and 64- bit builds. The output of readelf is like this:

-bash-3.00$ readelf -d certutil/Linux2.6_x86_glibc_PTH_DBG.OBJ/certutil

Dynamic section at offset 0x1eeb0 contains 30 entries:
  Tag        Type                         Name/Value
 0x00000001 (NEEDED)                     Shared library: [libssl3.so]
...
 0x0000000f (RPATH)                      Library rpath: [$ORIGIN/../lib]

-bash-3.00$ readelf -d certutil//Linux2.6_x86_64_glibc_PTH_64_DBG.OBJ/certutil

Dynamic section at offset 0x227a0 contains 30 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libssl3.so]
 ...
 0x000000000000000f (RPATH)              Library rpath: [$ORIGIN/../lib64]
 0x000000000000000c (INIT)               0x404788


which looks correct to me. Since the real test involves installing an rpm we'll need some help declaring it done. Both builds passed all.sh.

Comment 2

[Wan-Teh Chang]

Comment on attachment 210650 [details] [diff] [review]
proposed fix

The -z origin linker option shouldn't be necessary
and may not be supported by the GNU ld.  (GNU ld
ignores -z xxx options it doesn't support.)

Comment 3

[Wan-Teh Chang]

*** Bug 325789 has been marked as a duplicate of this bug. ***

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I thought I reassigned this to Neil last week.  :-/

Neil, Why is this change only being made if $(BUILD_SUN_PKG) equals 1  ?
If this change is correct for the platform, shouldn't it be present on
all builds for that platform, and not merely Sun's builds?

Neil, Christophe, please set a target fix release, and the appropriate priority.

Wan-Teh, do you feel strongly that the -z origin should be removed?

Comment 5

[Neil Williams]

Attachment: incorporates Wan-Teh's recommendation

I tested this patch and the results are the same for both patches. Although ld clearly knows about the -z origin option it seems to be its default behavior. (ld --version says GNU ld version 2.15.92.0.2 20040927.) Reviewers, which patch do we want to go with?

Comment 6

[Neil Williams]

(In reply to comment #4)
> I thought I reassigned this to Neil last week.  :-/
> 
> Neil, Why is this change only being made if $(BUILD_SUN_PKG) equals 1  ?
> If this change is correct for the platform, shouldn't it be present on
> all builds for that platform, and not merely Sun's builds?

The way this was reported indicated it was a problem with the way Sun packaged the 32- and 64- bit builds on Linux. In this packaging scheme the 64-bit binaries overwrite the 32-bit binaries when they are installed but the libraries remain in distinct directories. This required linking with RPATH values for each flavor of the binaries. Actually, adding RPATH values to the linked binaries would help any user who wished not to set LD_LIBRARY_PATH. Would this be desireable to other users besides Sun? If so I'll take the BUILD_SUN_PKG qualifier out.

> Neil, Christophe, please set a target fix release, and the appropriate
> priority.
> 
> Wan-Teh, do you feel strongly that the -z origin should be removed?
> 

Comment 7

[Wan-Teh Chang]

Comment on attachment 211086 [details] [diff] [review]
incorporates Wan-Teh's recommendation

Sorry, I spent some time doing research.  Here is what
I found. My experiments showed that the -z origin option
is not needed, and an expert in our company confirmed my
findings and said that it is only needed for Solaris
(I suspect by that he meant "for Solaris compatibility").
So please remove the -z origin option.

I am fine with not putting -rpath '$ORIGIN/../lib' etc.
inside ifdef BUILD_SUN_PKG.  This change is generally
useful.  On the other hand, if you need to use
-rpath /usr/lib/mps etc., those should be put inside
ifdef BUILD_SUN_PKG.

Please move the new code right before the HP-UX ia64
section later in the file.

You may want to do this on all platforms that use GNU
ld.

Do the shared libraries need to be linked with
-rpath '$ORIGIN'?

Comment 8

[Wan-Teh Chang]

Comment on attachment 211086 [details] [diff] [review]
incorporates Wan-Teh's recommendation

>+ifeq ($(USE_64), 1)
>+EXTRA_SHARED_LIBS += -Wl,-rpath,'$$ORIGIN/../lib64'
>+else
>+EXTRA_SHARED_LIBS += -Wl,-rpath,'$$ORIGIN/../lib'
>+endif

For a general-purpose fix (without the BUILD_SUN_PKG
ifdef), the USE_64 case should probably say:

EXTRA_SHARED_LIBS += -Wl,-rpath,'$$ORIGIN/../lib64:$$ORIGIN/../lib'

because I'm not sure if 'lib64' is the convention on
all 64-bit versions of Linux.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 211086 [details] [diff] [review]
incorporates Wan-Teh's recommendation

r=nelson

Comment 10

[Neil Williams]

Attachment: Added Wan-Teh's last suggestions, rpath added for all Linux builds

Thanks, Wan-Teh, for your comments. I will check this version in.

Comment 11

[Neil Williams]

Checking in cmd/platlibs.mk;
/cvsroot/mozilla/security/nss/cmd/platlibs.mk,v  <--  platlibs.mk
new revision: 1.44; previous revision: 1.43
done

Comment 12

[Wan-Teh Chang]

Neil, if you need this fix in NSS 3.11.1, you need to
check in your patch on the NSS_3_11_BRANCH, and set
the target milestone to 3.11.1.

Comment 13

[Neil Williams]

Checking in cmd/platlibs.mk;
/cvsroot/mozilla/security/nss/cmd/platlibs.mk,v  <--  platlibs.mk
new revision: 1.43.2.1; previous revision: 1.43
done