Code review comment for ~sergiodj/ubuntu/+source/libpcap:bug1865501-pkg-config

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

On Tuesday, June 30 2020, Christian Ehrhardt  wrote:

> Thanks for identifying the configure/make changes needed to get this working Sergio.
> The changelog looks well.
> The build log has a few warnings, but not related to your change.
> The PPA works as intended and shows no other impact.
>
> I double checked the backported patches for potential side-effects but found none \o/.
> Never the less the config changes make this way more noisy and intrusive than I'd have hoped :-/

Thanks for the review, Christian!

Indeed, I was also negatively surprised when I noticed that I'd need to
backport more than the trivial libpcap.pc.in change in order to make
this work.

> In the review I learned that libpcap had another - non standard - way to provide this data already.
> => /usr/bin/pcap-config
> This was retained going forward for compatibility:
> Focal:
> $ $ pcap-config --static --additional-libs --libs --cflags
> -I/usr/include -lpcap
> Bionic:
> root@b:~# pcap-config --static --libs --cflags --additional-libs
> -I/usr/include -lpcap
> Bionic + PPA:
> root@b:~# pcap-config --static --libs --cflags --additional-libs
> -I/usr/include -lpcap

Ah, that's correct, thanks for pointing it out! As you can see, part of
the changes I backported even patched the pcap-config script.

> +1 for the review on this.
>
> Let us see what the SRU Team thinks of "more intrusive than we hoped" + "there is a (non standard) alternative". Both arguments against the SRU, but no clear show stoppers.

Yep, fingers crossed.

As you said, pcap-config is non-standard, and, as far I have seen, not
really well documented, which explains why this bug was filed. Let's
hope the SRU team considers this justification sufficient :-).

Thanks again!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

« Back to merge proposal