rpath needs to be for some Linux builds

RESOLVED FIXED in 3.11.1

Status

NSS
Build
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Neil Williams, Assigned: Neil Williams)

Tracking

3.11
3.11.1
x86
SunOS

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

12 years ago
Created attachment 210650 [details] [diff] [review]
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.
Attachment #210650 - Flags: review?(nelson)

Comment 2

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

12 years ago
*** Bug 325789 has been marked as a duplicate of this bug. ***
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?
Assignee: wtchang → neil.williams
Version: unspecified → 3.11
(Assignee)

Comment 5

12 years ago
Created attachment 211086 [details] [diff] [review]
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?
(Assignee)

Comment 6

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

Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #211086 - Flags: superreview?(wtchang)
Attachment #211086 - Flags: review?(nelson)

Comment 7

12 years ago
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'?
Attachment #211086 - Flags: superreview?(wtchang) → superreview+

Comment 8

12 years ago
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 on attachment 211086 [details] [diff] [review]
incorporates Wan-Teh's recommendation

r=nelson
Attachment #211086 - Flags: review?(nelson) → review+
Attachment #210650 - Flags: review?(nelson)
(Assignee)

Comment 10

12 years ago
Created attachment 211196 [details] [diff] [review]
Added Wan-Teh's last suggestions, rpath added for all Linux builds

Thanks, Wan-Teh, for your comments. I will check this version in.
Attachment #210650 - Attachment is obsolete: true
Attachment #211086 - Attachment is obsolete: true
(Assignee)

Comment 11

12 years ago
Checking in cmd/platlibs.mk;
/cvsroot/mozilla/security/nss/cmd/platlibs.mk,v  <--  platlibs.mk
new revision: 1.44; previous revision: 1.43
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #211196 - Flags: review+

Comment 12

12 years ago
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.
Target Milestone: --- → 3.12
(Assignee)

Comment 13

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

Updated

12 years ago
Target Milestone: 3.12 → 3.11.1
You need to log in before you can comment on or make changes to this bug.