298455 - pkg.m4 is too conservative about pkg-config --libs

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Patrick Welche]

User-Agent:       Mozilla/5.0 (X11; U; NetBSD i386; en-US; rv:1.8b2) Gecko/20050620 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; NetBSD i386; en-US; rv:1.8b2) Gecko/20050620 Firefox/1.0+

pkg.m4 builds up LIBS compile variables from --libs-only-L and --libs-only-l so
as to avoid possible mishaps such as getting -Wl,--export-dynamic. This approach
is too conservative, as it means that it also gets rid of necessary flags such
as -Wl,-R/some/path. The patch I will attach once I find the right place on this
web front end takes the opposite approach: if you don't want
-Wl,--export-dynamic, then remove it.

The actually problem came about when xpidl wouldn't run, so it was impossible to
compile firefox. The reason was that xpidl couldn't find libIDL-2.so was because
the necessary -Wl,-R/usr/local/lib in

% pkg-config --libs libIDL-2.0
-Wl,-R/usr/local/lib -L/usr/local/lib -lIDL-2 -lglib-2.0 -lintl  

was removed by pkg.m4 while building up LIBIDL_LIBS.


Reproducible: Always

Comment 1

[Patrick Welche]

Attachment: as mentioned in original report

Comment 2

[Benjamin Smedberg]

Comment on attachment 187020 [details] [diff] [review]
as mentioned in original report

Cls, I have no understand of this code at all; this is all for you ;-)

Comment 3

[cls]

Comment on attachment 187020 [details] [diff] [review]
as mentioned in original report

AC_PROG_SED isn't a standard autoconf macro.  I don't see it in autoconf-2.13
which is what we currently require. 

Besides that, I'm not sure if we really need to fix this.  Historically, the
project has shied away from the use of -R/-rpath/etc for our final binaries, so
I'm not sure if we want to support them for our build tools either.   

The workaround is simple: set LD_LIBRARY_PATH before building.

Comment 4

[Patrick Welche]

I don't really want to go into arguments of LD_LIBRARY_PATH vs -rpath (I happen
to believe that -rpath is right), as the issue is with pkg.m4 removing any hope
of passing necessary flags in from pkg-config. -rpath is the example which hit me.

<aside>
(My personal preference would be not to use pkg-config and stick to autotools
completely, as libtool does know whether or not the system requires -rpath or
not. I'm not going there, as libtool v2 is far too tempting, and given the
autoconf 2.13 situation, I don't expect anyone to agree to using libtool)
</aside>

Now as to AC_PROG_SED, that can be jettisoned as I see that you use "sed"
throughout (e.g. in /configure.in), so I don't need to look for a working sed,
nor test its -e flag, so new patch attached.

Comment 5

[Patrick Welche]

Attachment: configure.in assumes sed is in path, so no need for AC_PROG_SED

Comment 6

[cls]

Comment on attachment 187042 [details] [diff] [review]
configure.in assumes sed is in path, so no need for AC_PROG_SED

That works but I still don't think we want it.	Deferring to dbaron.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 187042 [details] [diff] [review]
configure.in assumes sed is in path, so no need for AC_PROG_SED

This is OK with me, although we may be overestimating the intelligence or
knowledge of the people creating .pc files.  I don't understand the first half,
but I'll trust cls.

Comment 8

[Patrick Welche]

(In reply to comment #7)
> (From update of attachment 187042 [details] [diff] [review] [edit])
> This is OK with me, although we may be overestimating the intelligence or
> knowledge of the people creating .pc files.  I don't understand the first half,
> but I'll trust cls.

The first half is just formatting - in the first revision I had an AC_REQUIRE so
the need for reformatting was obvious, now it is irrelevant.

Re. the people creating .pc files, take as an example

  http://cvsweb.xfree86.org/cvsweb/xc/lib/Xft/xft.pc.in

I see they do the right thing with "${hardcode_libdir_flag_spec}" which you then
delete because of your overzealous "export-dynamic" removal...

Comment 9

[Patrick Welche]

Just under 3 months later after opening this bug, which even includes a working
patch, I thought I would try to compile today's mozilla. Naturally, as this
patch hasn't been applied, it fails with

gmake[4]: Entering directory `/temp/src/mozilla/xpcom/base'
/temp/src/mozilla/config/nsinstall -R -m 644 nsAgg.h nsAutoPtr.h nsCom.h nsDebug
Impl.h nsIAllocator.h nsIID.h nsISupportsObsolete.h nsTraceRefcntImpl.h nsWeakPt
r.h nsInterfaceRequestorAgg.h ../../dist/include/xpcom
/temp/src/mozilla/config/nsinstall -R -m 644 nsError.h nsISupportsBase.h nscore.
h ../../dist/include/xpcom
/temp/src/mozilla/config/nsinstall -R -m 644 nsError.h nsISupportsBase.h nscore.
h ../../dist/sdk/include
/temp/src/mozilla/config/nsinstall -R -m 644 nsIConsoleListener.idl nsIConsoleMe
ssage.idl nsIConsoleService.idl nsIErrorService.idl nsIException.idl nsIExceptio
nService.idl nsIVersionComparator.idl ../../dist/idl
nsIConsoleListener.idl
../../dist/bin/xpidl -m header -w -I. -I../../dist/idl -o _xpidlgen/nsIConsoleLi
stener nsIConsoleListener.idl
Shared object "libIDL-2.so.0" not found


I still don't see any argument against this patch, so must ask why hasn't it
been applied?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Because nobody ever asked somebody with checkin access to commit it once it had
reviews.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in.