Last Comment Bug 325682 - rpath needs to be for some Linux builds
: rpath needs to be for some Linux builds
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Build (show other bugs)
: 3.11
: x86 SunOS
: -- normal (vote)
: 3.11.1
Assigned To: Neil Williams
: Wan-Teh Chang
:
Mentors:
: 325789 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-02 18:00 PST by Neil Williams
Modified: 2006-02-08 18:43 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (969 bytes, patch)
2006-02-03 16:16 PST, Neil Williams
no flags Details | Diff | Splinter Review
incorporates Wan-Teh's recommendation (941 bytes, patch)
2006-02-07 17:19 PST, Neil Williams
nelson: review+
wtc: superreview+
Details | Diff | Splinter Review
Added Wan-Teh's last suggestions, rpath added for all Linux builds (802 bytes, patch)
2006-02-08 16:06 PST, Neil Williams
wtc: review+
Details | Diff | Splinter Review

Description Neil Williams 2006-02-02 18:00:53 PST
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 2006-02-03 16:16:16 PST
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.
Comment 2 Wan-Teh Chang 2006-02-03 16:54:50 PST
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 2006-02-03 16:56:12 PST
*** Bug 325789 has been marked as a duplicate of this bug. ***
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-02-06 17:47:02 PST
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 2006-02-07 17:19:47 PST
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?
Comment 6 Neil Williams 2006-02-07 17:31:20 PST
(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 2006-02-07 17:54:11 PST
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 2006-02-07 17:58:23 PST
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) 2006-02-08 09:25:14 PST
Comment on attachment 211086 [details] [diff] [review]
incorporates Wan-Teh's recommendation

r=nelson
Comment 10 Neil Williams 2006-02-08 16:06:38 PST
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.
Comment 11 Neil Williams 2006-02-08 16:13:19 PST
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 2006-02-08 16:39:10 PST
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 2006-02-08 18:19:49 PST
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

Note You need to log in before you can comment on or make changes to this bug.