289015 - nspr-config --libs produces wrong 64bit output on multiarch machines

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch from [e-mail]

Comment 2

[cls]

Does this patch assume that --libdir=/usr/lib64 is being passed in for 64bit builds?

Comment 3

[Wolfgang Rosenauer [:wolfiR]]

IMHO (because the patch is not from me ;-))
yes, but that's the usual behaviour for biarch architectures.

Comment 4

[Wan-Teh Chang]

Comment on attachment 179618 [details] [diff] [review]
Patch from [e-mail]

I think this patch is correct.	r=wtc.	Would appreciate
a second review by cls.

Comment 5

[Wan-Teh Chang]

Comment on attachment 179618 [details] [diff] [review]
Patch from [e-mail]

Requesting NSPRPUB_PRE_4_2_CLIENT_BRANCH checkin approval for
aviary1.1a and mozilla1.8b2.

I've reviewed this patch.  The Mozilla clients don't use this
file.

Comment 6

[Wan-Teh Chang]

Patch checked in on the NSPR trunk for NSPR 4.6.

Checking in nspr-config.in;
/cvsroot/mozilla/nsprpub/config/nspr-config.in,v  <--  nspr-config.in
new revision: 1.5; previous revision: 1.4
done

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 179618 [details] [diff] [review]
Patch from [e-mail]

a=asa

Comment 8

[Wan-Teh Chang]

Patch checked in on the NSPRPUB_PRE_4_2_CLIENT_BRANCH for
Mozilla 1.8 Beta 2.

Checking in nspr-config.in;
/cvsroot/mozilla/nsprpub/config/nspr-config.in,v  <--  nspr-config.in
new revision: 1.2.2.3; previous revision: 1.2.2.2
done

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #5)
> (From update of attachment 179618 [details] [diff] [review] [edit])
> I've reviewed this patch.  The Mozilla clients don't use this
> file.

Actually, they do.

http://lxr.mozilla.org/mozilla/source/configure.in#3138
http://lxr.mozilla.org/mozilla/source/configure.in#6582

My build doesn't work because nspr-config doesn't set libdir using the passed-in
--prefix, and @prefix@ is /usr/local. I'm not where the fault is yet.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mozilla's building nspr --with-dist-prefix=blahblah/mozilla/dist, so $prefix
stays at the default ... /usr/local. Someone tell me who's at fault here.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This patch fixes it for me. We need this about "now" because all our
tinderboxes are going red.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm checking this in to get our build unbusted, even though I'm not sure it's
the correct fix. Please discuss amongst yourselves.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, but of course I can't fix the bustage because I can't check into NSPR.

Comment 14

[Wan-Teh Chang]

I backed out the checkin.

Comment 15

[Wan-Teh Chang]

Attachment: fix 1 for build bustage

The default value of @libdir@ is ${exec_prefix}/lib.
So if --libdir is not specified, we have this line:

libdir=${exec_prefix}/lib

which will evaluate ${exec_prefix} right there, before
the nspr-config script's --prefix=foo or --exec-prefix=foo
option is processed.

So the first fix is to single-quote @libdir@ to prevent
evaluating ${exec_prefix} at the definition:

libdir='${exec_prefix}/lib'

But this means we will need to evaluate ${libdir} twice,
which is achieved with the expression `eval echo ${libdir}`.

I am not good at shell programming, so there may be a better
way to do this.

Comment 16

[Wan-Teh Chang]

Attachment: fix 2 for build bustage

The second fix is to move the definition of libdir
after we have processed the nspr-config's --prefix
and --exec-prefix options.

Let me know which fix you like better.	I like fix 1
better.

Comment 17

[Wan-Teh Chang]

Attachment: fix 3 for build bustage

This is a variant of fix 1.  I still like fix 1
better than this one, but am attaching this for
reference.

Comment 18

[cls]

Comment on attachment 182128 [details] [diff] [review]
fix 2 for build bustage

I don't believe either of these patches addresses the original problem that was
reported.  We cannot count on libdir='${exec_prefix}/lib'.  That's just the
default.  When NSPR is configured to use --libdir=/foo/lib64, then we'll have
to pass --libdir=/newfoo/lib64 to the nspr-config script to override the
default value as well.	

We could try supporting passing --libdir='\${exec_prefix}/lib64' to NSPR's
configure but I think that will get too complicated to support in the long run.

Comment 19

[cls]

Attachment: v4

In this version of the patch, we do not set the exec_prefix, includedir &
libdir values to the default until the commandline args have been evaluated. 
This allows --prefix to change the results of --cflags & --libs as expected if
no other args are passed to configure or if they are passed in like
--libdir='${exec_prefix}/lib64 .  If --libdir=/foo/lib64 is given, then
--libdir must also be given to nspr-config to override the value set at
configure time.

Comment 20

[cls]

Attachment: moz v1

This patch fixes the Mozilla callers of nspr-config.

Comment 21

[Wan-Teh Chang]

Comment on attachment 182456 [details] [diff] [review]
v4

r=wtc.	Thank you for this patch, cls.

Does this mean 'prefix' can't depend on any other variable?

Comment 22

[Wan-Teh Chang]

Comment on attachment 182476 [details] [diff] [review]
moz v1

>-        NSPR_LIBS='`$(DEPTH)/nsprpub/config/nspr-config --prefix=$(DIST) --libs`'
>+        NSPR_LIBS='`$(DEPTH)/nsprpub/config/nspr-config --prefix=$(DIST) --libdir=$(DIST)/lib --libs`'

Question: if people configure Mozilla with --libdir=/foo/lib64, do we still
want to pass --libdir=$(DIST)/lib to nspr-config here?

Is there documentation on these foo-config files?

Comment 23

[cls]

prefix shouldn't depend upon anything else.  It's usually the value passed in
via --prefix to configure.

Yes, if people configure Mozilla (and NSPR) with --libdir=/foo/lib64, we still
want to use --libdir=$(DIST)/lib when calling nspr-config to set NSPR_LIBS.  The
Mozilla build system is hardcoded to look for the libraries in $(DIST)/lib.  If
we didn't change the libdir when calling nspr-config, then NSPR_LIBS would
contain the --libdir passed to configure and we don't want to use that when
building against the local copy of NSPR.

I'm not aware of any written standard for the foo-config scripts.  Most seem to
be derived from glib-config/gtk-config.  I believe there's a drive to replace
them with pkg-config files anyway.

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 182476 [details] [diff] [review]
moz v1

a=shaver

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 182456 [details] [diff] [review]
v4

a=shaver

Comment 26

[cls]

The patches have been checked in on the Mozilla & NSPR trunks.