Bug 13497

Created attachment 12914 [details] [review]
Query pkg-config for "libpci" in configure script

Attached a patch that will make configure query pkg-config for the libpci package, and fill the PCIUTILS_* variables with appropriate information.

Created attachment 12915 [details] [review]
Patch to rhd_conntest Makefile.am to use the pkg-config substitutions for libpci

Attached the necessary code to have the Makefile.am generate a Makefile for rhd_conntest that will properly source in the substitutions to build the program if libpci is registered in pkg-config's database.

(In reply to comment #2)
> Patch to rhd_conntest Makefile.am to use the pkg-config substitutions for
> libpci
> 
> Attached the necessary code to have the Makefile.am generate a Makefile for
> rhd_conntest that will properly source in the substitutions to build the
> program if libpci is registered in pkg-config's database.

-rhd_conntest_LDADD   = -lz -lpci
+rhd_conntest_LDADD   = -lz @PCIUTILS_LIBS@ -lpci

Shouldn't this read

+rhd_conntest_LDADD   = @PCIUTILS_LIBS@

? It is the .pc file's job to know whether or not -lz is needed, and where to find the libraries.

I'd be a bit hesitant to add this to radeonhd proper as long as the pciutils people actually ship their own .pc file, or the community of package creators for all systems agree on what it is supposed to be called like.

If pciutils shipped the .pc file, it would be a no-brainer.

I mean... the CFLAGS/LDFLAGS workaround does work flawlessly, so it's not as if this actually prevented you from building radeonhd.

(In reply to comment #3)
> (In reply to comment #2)
> > Patch to rhd_conntest Makefile.am to use the pkg-config substitutions for
> > libpci
> > 
> > Attached the necessary code to have the Makefile.am generate a Makefile for
> > rhd_conntest that will properly source in the substitutions to build the
> > program if libpci is registered in pkg-config's database.
> 
> -rhd_conntest_LDADD   = -lz -lpci
> +rhd_conntest_LDADD   = -lz @PCIUTILS_LIBS@ -lpci
> 
> Shouldn't this read
> 
> +rhd_conntest_LDADD   = @PCIUTILS_LIBS@
> 
> ? It is the .pc file's job to know whether or not -lz is needed, and where to
> find the libraries.
> 
> I'd be a bit hesitant to add this to radeonhd proper as long as the pciutils
> people actually ship their own .pc file, or the community of package creators
> for all systems agree on what it is supposed to be called like.
> 
> If pciutils shipped the .pc file, it would be a no-brainer.
> 
> I mean... the CFLAGS/LDFLAGS workaround does work flawlessly, so it's not as if
> this actually prevented you from building radeonhd.
> 

I agree with you, that is how it *should* read. However, the libpci.pc file was only added as of pciutils 2.2.6, and I was trying to avoid breaking the build for people who didn't have this version installed. You'll notice in the configure.ac that I let the configure fall through with only a warning when the pkg-config lookup for "libpci" fails. It then attempts to still use the same method as before to check for pci/pci.h.

If you want to do it now, then go ahead and change the patch to read the more appropriate: 
+rhd_conntest_LDADD   = @PCIUTILS_LIBS@

When the newest pciutils filters its way down, this should work for everybody. This change has already made its way into Gentoo portage.

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)

> > -rhd_conntest_LDADD   = -lz -lpci
> > +rhd_conntest_LDADD   = -lz @PCIUTILS_LIBS@ -lpci
> > 
> > Shouldn't this read
> > 
> > +rhd_conntest_LDADD   = @PCIUTILS_LIBS@

> I agree with you, that is how it *should* read. However, the libpci.pc file was
> only added as of pciutils 2.2.6, and I was trying to avoid breaking the build
> for people who didn't have this version installed. You'll notice in the
> configure.ac that I let the configure fall through with only a warning when the
> pkg-config lookup for "libpci" fails. It then attempts to still use the same
> method as before to check for pci/pci.h.

OK, if that pkg-config libpci check fails, I now just set PCIUTILS_LIBS="-lpci -lz" (like libpci.pc does) and proceed with the compile/link check. Then I'll never get "-lpci -lpci".

This should use libpci.pc if present, and totally fall back to current behaviour if not.

> If you want to do it now, then go ahead and change the patch to read the more
> appropriate: 
> +rhd_conntest_LDADD   = @PCIUTILS_LIBS@
> 
> When the newest pciutils filters its way down, this should work for everybody.
> This change has already made its way into Gentoo portage.
> 

I have submitted your patch and one with my changes to the radeonhd mailing list.

> 
> I have submitted your patch and one with my changes to the radeonhd mailing
> list.
> 

I saw the patch in the submission message, it looks pretty good to me. Should we also post the final patch here (and obsolete the above(s))?

Committed and pushed,

Thanks a lot guys :)