245518 - NSS tools should have an rpath of /usr/lib/mps/secv1

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

Several NSS tools (certutil, modutil, and a couple more ) get bundled with
Solaris and need to work even if LD_LIBRARY_PATH is not set. The NSS libraries
are loaded in /usr/lib/mps/secv1 . The solution is to link the tools with -R
/usr/lib/mps/secv1 .

Note that if LD_LIBRARY_PATH is set, it takes precedence over the -R in the
executable, so it is still possible to use the tools with an alternate version
of NSS if desired.

Comment 1

[Julien Pierre]

Attachment: set rpath as needed when linking NSS tools on Solaris

This patch also sets -R $ORIGIN and -R $ORIGIN/../lib since ../lib it is a very
common location for the NSS libraries relative to the tools.

For 64-bit versions, it sets rpath to / usr/lib/mps/secv1/sparcv9, which is
where we bundle the 64-bit NSPR/NSS binaries in Solaris.

Comment 2

[Wan-Teh Chang]

Comment on attachment 149969 [details] [diff] [review]
set rpath as needed when linking NSS tools on Solaris

>+EXTRA_SHARED_LIBS += -R '$$ORIGIN/../lib' -R '$$ORIGIN/..'

The $ORIGIN/.. path seems to be specific to the NSS build
intended to be installed under /usr/lib/mps.  See my
comment below.

Did you mean to say -R '$$ORIGIN', which would match your
comment 1?

>+ifeq ($(USE_64), 1)
>+EXTRA_SHARED_LIBS += -R '/usr/lib/mps/secv1/sparcv9'
>+else
>+EXTRA_SHARED_LIBS += -R '/usr/lib/mps/secv1'
>+endif

This path is only right for the NSS build intended
to be installed under /usr/lib/mps.  We should not
do this in general.  We should invent a new make
variable BUILD_SUN_PKG or some other build option
to indicate that we are building NSS this way.
(Note: BUILD_SUN_PKG was used on the
NSPRPUB_RELEASE_4_1_BRANCH to handle a similar
issue.)

Comment 3

[Saul Edwards]

Attachment: add BUILD_SUN_PKG variable to original patch.

This version uses BUILD_SUN_PKG for the /usr/lib/mps rpath addition as
suggested by Wan-Teh.  I tested on Solaris with and without BUILD_SUN_PKG=1.

Julien, please confirm that you meant ORIGIN and not ORIGIN/.. in your original
patch.	ORIGIN/.. is dangerous, especially with the 64-bit version in the
sparcv9 directory.

Comment 4

[Wan-Teh Chang]

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

Comment 5

[Wan-Teh Chang]

Comment on attachment 150642 [details] [diff] [review]
add BUILD_SUN_PKG variable to original patch.

This patch is good.  r=wtc.

Note that I don't know whether you want -R '$$ORIGIN/..'
or -R '$$ORIGIN'.  Kirk Erickson's patch in bug 217376
also says -R '$$ORIGIN/..', and he explained why:

  This will make installed Solaris packages work, since
  the libs install in /usr/lib/mps, and the tools into
  /usr/lib/mps/bin

If it is -R '$$ORIGIN/..' that you want, it should also
be moved inside the BUILD_SUN_PKG block.

Comment 6

[Wan-Teh Chang]

Comment on attachment 150642 [details] [diff] [review]
add BUILD_SUN_PKG variable to original patch.

Saul, Julien,

Another comment: I am wondering if the order between
-R '$$ORIGIN/../lib' and -R '$$ORIGIN' matters.

Comment 7

[Wan-Teh Chang]

Comment on attachment 150642 [details] [diff] [review]
add BUILD_SUN_PKG variable to original patch.

It seems better to search in $ORIGIN before $ORIGIN/../lib.
Since on Unix people don't usually install .so's in the
same directory as the executable programs, it's not clear
which ordering is right.

Comment 8

[Saul Edwards]

(In reply to comment #5)

There is no such thing as /usr/lib/mps/bin any more... we meant $$ORIGIN


As for the ordering, I don't have a preference because we never install binaries
with libraries.  Anyone else?

Comment 9

[Julien Pierre]

Comment on attachment 150642 [details] [diff] [review]
add BUILD_SUN_PKG variable to original patch.

Looks good. Please checking for 3.9.2 . Thanks !

Comment 10

[Wan-Teh Chang]

Comment on attachment 150642 [details] [diff] [review]
add BUILD_SUN_PKG variable to original patch.

You might want to remove -R '$$ORIGIN' to be
conservative.  Apparently nobody asks for it.

Comment 11

[Julien Pierre]

Christophe,

Please start building with the BUILD_SUN_PKG=1 make variable . Thanks !

Comment 12

[Saul Edwards]

Attachment: Final patch without $ORIGIN, checked in

Checked in with $ORIGIN/../lib and not $ORIGIN, as suggested by Wan-Teh.

Tip:

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

NSS_3_9_BRANCH:

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

Comment 13

[Wan-Teh Chang]

Thanks, Saul.  If we ever add $ORIGIN back, I've formed
my opinion: it should go before $ORIGIN/../lib because
$ORIGIN implies a tighter binding than $ORIGIN/../lib.

Comment 14

[Julien Pierre]

We also need to have /usr/lib/mps since the secv1 location is being obsoleted .
Reopening bug.

Comment 15

[Julien Pierre]

Attachment: add mps location. incremental patch

Comment 16

[Julien Pierre]

Comment on attachment 158370 [details] [diff] [review]
add mps location. incremental patch

Saul, please review. If this looks ok, could you check this patch in to the tip
and NSS_3_9_BRANCH ?

Thanks .

Comment 17

[Julien Pierre]

Actually, I think the original patch is busted. The Solaris linker only
considers one instance of "-R" - the last one . So we can't use += . 

Comment 18

[Saul Edwards]

Comment on attachment 158370 [details] [diff] [review]
add mps location. incremental patch

RPATHs cannot be "added" (+=) because compilers will only accept the first -R
option.

Comment 19

[Saul Edwards]

Attachment: Compact the RPATH option for each case, add /usr/lib/mps for Sun

Tested BUILD_SUN_PKG on and off, using Solaris.

Comment 20

[Wan-Teh Chang]

Comment on attachment 159228 [details] [diff] [review]
Compact the RPATH option for each case, add /usr/lib/mps for Sun

Saul, could you adjust your patch so that it
uses ifeq instead of ifneq with BUILD_SUN_PKG?
It's easier for me to understand that way.  Thanks.

r=wtc.

Comment 21

[Saul Edwards]

Attachment: Final patch, checked in

Comment 22

[Saul Edwards]

Checked in:

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

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