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

RESOLVED FIXED in 4.6

Status

defect
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: caillon, Assigned: wtc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Comment 2

14 years ago
Does this patch assume that --libdir=/usr/lib64 is being passed in for 64bit builds?
OS: Linux → All
Hardware: PC → All
IMHO (because the patch is not from me ;-))
yes, but that's the usual behaviour for biarch architectures.
Assignee

Comment 4

14 years ago
Comment on attachment 179618 [details] [diff] [review]
Patch from rstrode@redhat.com

I think this patch is correct.	r=wtc.	Would appreciate
a second review by cls.
Attachment #179618 - Flags: superreview?(cls)
Attachment #179618 - Flags: review?(wtchang)
Attachment #179618 - Flags: review+
Assignee

Comment 5

14 years ago
Comment on attachment 179618 [details] [diff] [review]
Patch from rstrode@redhat.com

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.
Attachment #179618 - Flags: approval1.8b2?
Attachment #179618 - Flags: approval-aviary1.1a?

Updated

14 years ago
Attachment #179618 - Flags: superreview?(cls) → superreview+
Assignee

Comment 6

14 years ago
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
Status: NEW → ASSIGNED
Target Milestone: --- → 4.6

Comment 7

14 years ago
Comment on attachment 179618 [details] [diff] [review]
Patch from rstrode@redhat.com

a=asa
Attachment #179618 - Flags: approval1.8b2?
Attachment #179618 - Flags: approval1.8b2+
Attachment #179618 - Flags: approval-aviary1.1a?
Attachment #179618 - Flags: approval-aviary1.1a+
Assignee

Comment 8

14 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(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.
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.
Posted patch fix (obsolete) — Splinter Review
This patch fixes it for me. We need this about "now" because all our
tinderboxes are going red.
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.
Ah, but of course I can't fix the bustage because I can't check into NSPR.
Assignee

Comment 14

14 years ago
I backed out the checkin.
Assignee

Updated

14 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 15

14 years ago
Posted patch fix 1 for build bustage (obsolete) — Splinter Review
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.
Assignee

Updated

14 years ago
Attachment #182122 - Attachment is obsolete: true
Attachment #182127 - Flags: review?(cls)
Assignee

Comment 16

14 years ago
Posted patch fix 2 for build bustage (obsolete) — Splinter Review
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.
Attachment #182128 - Flags: review?(cls)
Assignee

Comment 17

14 years ago
Posted patch fix 3 for build bustage (obsolete) — Splinter Review
This is a variant of fix 1.  I still like fix 1
better than this one, but am attaching this for
reference.

Updated

14 years ago
Attachment #182127 - Flags: review?(cls) → review-

Comment 18

14 years ago
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.
Attachment #182128 - Flags: review?(cls) → review-

Comment 19

14 years ago
Posted patch v4Splinter Review
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.
Attachment #179618 - Attachment is obsolete: true
Attachment #182127 - Attachment is obsolete: true
Attachment #182128 - Attachment is obsolete: true
Attachment #182130 - Attachment is obsolete: true
Attachment #182456 - Flags: review?(wtchang)

Comment 20

14 years ago
Posted patch moz v1Splinter Review
This patch fixes the Mozilla callers of nspr-config.
Attachment #182476 - Flags: review?(roc)
Assignee

Comment 21

14 years ago
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?
Attachment #182456 - Flags: review?(wtchang) → review+
Assignee

Comment 22

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

14 years ago
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.

Updated

14 years ago
Attachment #182456 - Flags: approval1.8b2?

Updated

14 years ago
Attachment #182476 - Flags: approval1.8b2?
Comment on attachment 182476 [details] [diff] [review]
moz v1

a=shaver
Attachment #182476 - Flags: approval1.8b2? → approval1.8b2+
Comment on attachment 182456 [details] [diff] [review]
v4

a=shaver
Attachment #182456 - Flags: approval1.8b2? → approval1.8b2+

Comment 26

14 years ago
The patches have been checked in on the Mozilla & NSPR trunks.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.