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
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
--
http://trac.haqistan.net | att...@stalphonsos.com | 0xE6CC1EDB
cvs server: Diffing .
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";
}
cvs server: Diffing OpenBSD
cvs server: Diffing .
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:
cvs server: Diffing pcdir
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
