attila <att...@stalphonsos.com> writes:
> Jasper Lievisse Adriaanse <jas...@openbsd.org> writes:
>
>> On Thu, Aug 13, 2015 at 04:09:02PM -0500, attila wrote:
>>>
>>> Jasper Lievisse Adriaanse <jas...@openbsd.org> writes:
>>>
>>> > On Tue, Aug 11, 2015 at 01:20:24PM -0500, attila wrote:
>>> >> Hello tech@,
>>> >>
>>> >> On the 6 Aug snap I ran into this issue:
>>> >>
>>> >> $ pkg_info | grep libevent
>>> >> libevent-2.0.22 event notification library
>>> >> $ pkg-config --atleast-version=2.0.1 libevent && echo yes
>>> >> Argument "22-stabl" isn't numeric in numeric eq (==) at
>>> >> /usr/bin/pkg-config line 662.
>>> >> yes
>>> >>
>>> >> So it works but spits out that error. The issue is that the libevent
>>> >> port reports its version as 2.0.22-stable, which does not follow the
>>> >> convention used by the compare() sub in pkg-config. The attached
>>> >> patch gets rid of the error albeit in a very pointilistic way; perhaps
>>> >> a more general solution is desired, but I'm not sure what the best
>>> >> approach is, especially given that there is something called
>>> >> pkg-config available pretty much everywhere but based on very
>>> >> different implementations...
>>> >>
>>> >> It doesn't appear as though my patch introduces any regressions,
>>> >> although I'm still sussing out how the regression tests work so I'm
>>> >> not sure if I'm doing anything wrong... it appears that five of
>>> >> pkg-config's regression tests fail regardless of my patch:
>>> >>
>>> >> FAIL usr.bin/pkg-config/static-cflags2
>>> >> FAIL usr.bin/pkg-config/static-libs2
>>> >> FAIL usr.bin/pkg-config/static-libs3
>>> >> FAIL usr.bin/pkg-config/static-libs4
>>> >> FAIL usr.bin/pkg-config/missing-req-2
>>> >>
>>> >> In all cases it looks like the difference is ordering, except for the
>>> >> last where two errors are produced but only one is expected. I'll
>>> >> work on a patch to fix these failures next.
>>> > That's indeed the case and patches to fix that are welcome.
>>> >
>>> >> Feedback, comments most welcome.
>>> > Could you please add regress tests for this issue you're solving too?
>>>
>>> This turned out to be not so much difficult as cognitive
>>> dissonance-inducing. The details are numbingly boring but I'll
>>> happily discuss if there's an issue. In the end I have done what you
>>> asked.
>>>
>>> The first patch attached modifies pkg-config itself in the following
>>> ways:
>>>
>>> * Adds a (looks like libevent-specific) fix to compare() so that
>>> libevent's version number in e.g. libevent.pc (2.0.22-stable) does
>>> not cause warnings;
>>> * Hoists the code that redirected stderr -> stdout when
>>> --errors-to-stdout was specified from say_msg() into the main
>>> program, so that such warnings (if they ever appear again) appear in
>>> the .got files produced by regression tests;
>>> * Cleans up a couple stylistic nits (add a space between ")" and "{")
>>> in a few places to be consistent with how all the other Perl code
>>> looks.
>>>
>>> The second patch attached cleans up the failing regression tests.
>>> I've verified that our pkg-config produces the same output as the(a?)
>>> one on at least Linux given our test cases, and manually examined all
>>> the output. It looks right to me.
>>>
>>> As always feedback, comments, beatings most welcome.
>> Could you please send the spacing as a separate diff? I'm fine with that
>> going
>> in first, but it's making it less obvious where the real goodies are ;-)
>>
>
> This is reminding me of a Troma movie: I keep sticking a fork in its
> eye but the beast just won't go down...
>
> Forget my previous patches. There are two new patches attached.
>
> First patch in src/usr.bin/pkg-config does the following:
>
> 1. Factors out the regular expression used to check a version number
> for validity into a variable so that we have the same idea about
> this everywhere;
> 2. Uses this regexp in the main loop;
> 3. Uses this regexp in compare() and adjusts the captured subexp
> variables used to get the components of the optional suffix
> e.g. "alpha1";
> 4. Hoists the stderr->stdout redirection code to early in the main
> program for the same reason my previous patch did (which, BTW,
> helped me find bugs in my patch after earlier versions of it caused
> other regression tests to fail, so... yay?).
>
> Second patch in src/regress/usr.bin/pkg-config does the following:
>
> 1. Cleans up the broken tests that were expecting the wrong thing;
> 2. Adds five new tests to make sure I didn't screw anything up,
> which includes two new files in pcdir.
>
> If this patch is acceptable I'll submit another with whitespace
> cleanups.
>
> As usual feedback, comments most welcome.
>
> Pax, -A
Sigh. My patches got mangled by emacs in another new and exciting
way. Here they are inline:
Index: pkg-config
===================================================================
RCS file: /cvs/src/usr.bin/pkg-config/pkg-config,v
retrieving revision 1.85
diff -u -p -r1.85 pkg-config
--- pkg-config 17 Nov 2014 22:16:23 -0000 1.85
+++ pkg-config 14 Aug 2015 19:11:33 -0000
@@ -48,6 +48,10 @@ my $found_uninstalled = 0;
my $version = '0.27.1'; # pretend to be this version of pkgconfig
+# Regexp that captures our idea of a version number which accomodates
+# version numbers found in practice in e.g. the ports tree:
+my $vers_regexp = qr/^([\d\.]+)(|(-stable|rc|beta|alpha|[a-zA-Z])(|\d+))*$/;
+
my %configs = ();
setup_self();
@@ -109,6 +113,17 @@ GetOptions( 'debug' =>
\$mode{debug},
'define-variable=s' => $variables,
);
+# If --errors-to-stdout was given, close STDERR (to be safe),
+# then dup the output to STDOUT and delete the key from %mode so we
+# won't keep checking it. STDERR stays dup'ed.
+# We do this early so any other warnings also get redirected, since
+# they might indicate a bug that a regression test should catch.
+if ($mode{estdout}) {
+ close(STDERR);
+ open(STDERR, ">&STDOUT") or die "Can't dup STDOUT: $!";
+ delete($mode{estdout});
+}
+
# Unconditionally switch to static mode on static arches as --static
# may not have been passed explicitly, but we don't want to re-order
# and simplify the libs like we do for shared architectures.
@@ -166,7 +181,7 @@ while (@ARGV){
my $op = undef;
my $v = undef;
if (@ARGV >= 2 && $ARGV[0] =~ /^[<=>!]+$/ &&
- $ARGV[1] =~ /^[\d\.]+[\w\.]*$/) {
+ $ARGV[1] =~ $vers_regexp) {
$op = shift @ARGV;
$v = shift @ARGV;
}
@@ -623,30 +638,23 @@ sub compare
return 0 if ($a eq $b);
# is there a valid non-numeric suffix to deal with later?
- # accepted are (in order): a(lpha) < b(eta) < rc < ' '.
+ # accepted are listed in $vers_regexp, above, and are compared
+ # in alphabetical order, so e.g. 'beta' beats 'alpha'.
# suffix[0] is the 'alpha' part, suffix[1] is the '1' part in 'alpha1'.
- if ($a =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
- say_debug("valid suffix $1$2 found in $a$1$2.");
- $suffix_a[0] = $1;
- $suffix_a[1] = $2;
- }
-
- if ($b =~ s/(rc|beta|b|alpha|a)(\d+)$//) {
- say_debug("valid suffix $1$2 found in $b$1$2.");
- $suffix_b[0] = $1;
- $suffix_b[1] = $2;
- }
-
- # The above are standard suffixes; deal with single alphabetical
- # suffixes too, e.g. 1.0.1h
- if ($a =~ s/([a-zA-Z]){1}$//) {
- say_debug("valid suffix $1 found in $a$1.");
- $suffix_a[0] = $1;
- }
-
- if ($b =~ s/([a-zA-Z]){1}$//) {
- say_debug("valid suffix $1 found in $b$1.");
- $suffix_b[0] = $1;
+ # $2 will be the optional separator (currently nothing or dash)
+ if ($a =~ $vers_regexp && ($3 || $4)) {
+ $a = $1;
+ say_debug("valid suffix $3$4 found in $a$3$4");
+ $suffix_a[0] = $3;
+ $suffix_a[0] =~ s/^-//;
+ $suffix_a[1] = $4 || 0;
+ }
+ if ($b =~ $vers_regexp && ($3 || $4)) {
+ $b = $1;
+ say_debug("valid suffix $3$4 found in $b$3$4");
+ $suffix_b[0] = $3;
+ $suffix_b[0] =~ s/^-//;
+ $suffix_b[1] = $4 || 0;
}
my @a = split(/\./, $a);
@@ -823,15 +831,6 @@ sub say_warning
sub say_msg
{
my $str = shift;
-
- # If --errors-to-stdout was given, close STDERR (to be safe),
- # then dup the output to STDOUT and delete the key from %mode so we
- # won't keep checking it. STDERR stays dup'ed.
- if ($mode{estdout}) {
- close(STDERR);
- open(STDERR, ">&STDOUT") or die "Can't dup STDOUT: $!";
- delete($mode{estdout});
- }
print STDERR $str . "\n";
}
Index: Makefile
===================================================================
RCS file: /cvs/src/regress/usr.bin/pkg-config/Makefile,v
retrieving revision 1.49
diff -u -p -r1.49 Makefile
--- Makefile 2 Nov 2014 10:34:52 -0000 1.49
+++ Makefile 14 Aug 2015 19:14:19 -0000
@@ -38,6 +38,10 @@ REGRESS_TARGETS=cmp-vers1-1 \
cmp-vers7-2 \
cmp-vers7-3 \
cmp-vers7-4 \
+ cmp-vers8-1 \
+ cmp-vers8-2 \
+ cmp-vers8-3 \
+ cmp-vers8-4 \
corrupt1 \
corrupt2 \
corrupt3 \
@@ -79,7 +83,8 @@ REGRESS_TARGETS=cmp-vers1-1 \
variables-2 \
variables-3 \
variables-4 \
- variables-5
+ variables-5 \
+ dash-stable
PKG_CONFIG?= pkg-config
PCONFIG = PKG_CONFIG_PATH=${.CURDIR}/pcdir/ ${PKG_CONFIG}
@@ -343,6 +348,30 @@ cmp-vers7-4:
@${VPCONFIG} "vers3 > 1.0.1"
@diff -u ${WANT} ${GOT}
+cmp-vers8-1:
+ # Test -stable version (2.0.22-stable >= 2.0.22)
+ @touch ${WANT}
+ @${VPCONFIG} "dash-stable > 2.0.22"
+ @diff -u ${WANT} ${GOT}
+
+cmp-vers8-2:
+ # Test -stable version (2.0.22-stable >= 2.0.22beta)
+ @touch ${WANT}
+ @${VPCONFIG} "dash-stable > 2.0.22beta"
+ @diff -u ${WANT} ${GOT}
+
+cmp-vers8-3:
+ # Test -stable version (2.0.22-stable >= 2.0.22beta2)
+ @touch ${WANT}
+ @${VPCONFIG} "dash-stable > 2.0.22beta2"
+ @diff -u ${WANT} ${GOT}
+
+cmp-vers8-4:
+ # Test -stable version (2.0.22rc1 < 2.0.22-stable)
+ @touch ${WANT}
+ @${VPCONFIG} "rc1 < 2.0.22-stable"
+ @diff -u ${WANT} ${GOT}
+
# Tests for various environment variables
builddir:
# Test PKG_CONFIG_TOP_BUILD_DIR
@@ -375,7 +404,7 @@ static-cflags1:
static-cflags2:
# Test grabbing Cflags (with Requires.private)
- @echo "-I/usr/local/include/foo -I/usr/local/include" > ${WANT}
+ @echo "-I/usr/local/include -I/usr/local/include/foo" > ${WANT}
@${VPCONFIG} --cflags --static static2
@diff -u ${WANT} ${GOT}
@@ -387,19 +416,19 @@ static-libs1:
static-libs2:
# Test grabbing Libs.private from Requires in order
- @echo "-L/usr/local/lib -lc -lm -ll -lutil -lz" > ${WANT}
+ @echo "-L/usr/local/lib -lutil -lz -lc -lm -ll" > ${WANT}
@${VPCONFIG} --libs --static static2
@diff -u ${WANT} ${GOT}
static-libs3:
# Test grabbing Libs.private from Requires.private in order
- @echo "-L/tmp/lib -L/tmp/lib/foo -L/usr/local/lib -lbaz\ quux -lc -lm
-ll -lutil -lz" > ${WANT}
+ @echo "-L/usr/local/lib -L/tmp/lib -L/tmp/lib/foo -lutil -lz -lc -lm
-ll -lbaz\ quux" > ${WANT}
@${VPCONFIG} --libs --static static3
@diff -u ${WANT} ${GOT}
static-libs4:
# Test Requires.private
- @echo "-L/public-dep/lib -L/private-dep/lib -L/requires-test/lib
-lpublic-dep -lprivate-dep -lrequires-test" > ${WANT}
+ @echo "-L/requires-test/lib -L/private-dep/lib -L/public-dep/lib
-lrequires-test -lprivate-dep -lpublic-dep" > ${WANT}
@${VPCONFIG} --libs --static requires-test
@diff -u ${WANT} ${GOT}
@@ -419,7 +448,8 @@ missing-req-1:
missing-req-2:
# Test for missing packages in Requires (cflags)
- @echo "Package nonexisting was not found in the pkg-config search path"
> ${WANT}
+ @echo "Package nonexisting2 was not found in the pkg-config search
path" > ${WANT}
+ @echo "Package nonexisting was not found in the pkg-config search path"
>> ${WANT}
@if ${VPCONFIG} --cflags missing-req; then false; fi
@diff -u ${WANT} ${GOT}
@@ -553,6 +583,12 @@ variables-5:
# Test --variable
@echo ' -lfoo-1' > ${WANT}
@${VPCONFIG} --libs variables
+ @diff -u ${WANT} ${GOT}
+
+dash-stable:
+ # Test --atleast-version=2.0.1 against 2.0.22-stable
+ @touch ${WANT}
+ @${VPCONFIG} --atleast-version=2.0.1 dash-stable
@diff -u ${WANT} ${GOT}
clean:
Index: pcdir/dash-stable.pc
===================================================================
RCS file: pcdir/dash-stable.pc
diff -N pcdir/dash-stable.pc
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ pcdir/dash-stable.pc 14 Aug 2015 19:14:19 -0000
@@ -0,0 +1,3 @@
+Name: dash-stable
+Description: pkg-config(1) regress file
+Version: 2.0.22-stable
Index: pcdir/rc1.pc
===================================================================
RCS file: pcdir/rc1.pc
diff -N pcdir/rc1.pc
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ pcdir/rc1.pc 14 Aug 2015 19:14:19 -0000
@@ -0,0 +1,3 @@
+Name: rc1
+Description: pkg-config(1) regress file
+Version: 2.0.22rc1
