Bug#740607: lintian: Please support build-profiles
Hi,
Quoting Niels Thykier (2014-03-03 22:05:31)
> Thanks for looking into this.
and thanks for responding so quickly :)
> > +Tag: invalid-restriction-label-in-source-relation
> > +Severity: important
> > +Certainty: possible
> > +Info: The restriction list in the source relation includes a term with
> > + an unknown label. The only allowed labels are "stage1", "stage2", "notest"
> > + and "cross".
> > +
>
> This is inconsistent when the check, which only seem to be allowing
> "stage1" and "notest"
Thanks for spotting this - fixed now.
> Usually, we don't bother doing "conflict" testing. At least, we skip it
> for regular debhelper dependencies and many other cases.
I noticed. Does that mean that you want me to remove the conflict checking?
Since it's already done now it does not create more work for me to leave it
there. Maybe you want me to combine dependency and conflict testing into one
single tag each?
> > diff --git a/checks/fields.pm b/checks/fields.pm
> > index aeee252..2cae275 100644
> > --- a/checks/fields.pm
> > +++ b/checks/fields.pm
> > @@ -730,7 +730,7 @@ sub run {
> > && $known_obsolete_emacs{$alternatives[0]->[0]});
> >
> > for my $part_d (@alternatives) {
> > - my ($d_pkg, $d_march, $d_version, $d_arch, $rest,
> > + my ($d_pkg, $d_march, $d_version, $d_arch, $d_restr, $rest,
> > $part_d_orig)
> > = @$part_d;
> >
>
> If $d_restr is not used in the scope, please consider replacing it with
> "undef" to signal that. (The diff suggests it is unused).
Correct, just as architecture restrictions, restriction lists are not found in
binary package stanzas. I usually copy the implementation of architecture
restrictions when I prepare patches for restriction lists. I set $d_restr to
undef but note, that then (for consistency) $d_arch should be set to undef too
because it is also not used.
> > + for my $restr (@{$d_restr})
> > { + my $dotcount = () = $restr =~ /\./g;
>
> You probably want:
> my $dotcount = $restr =~ tr/.//;
Is there a difference between the former and the latter? I replaced it by the
latter as you suggested as it seems to do the same.
> > + my ($ns, $label) = split(/\./, $restr, 2);
> > + tag 'invalid-restriction-namespace-in-source-relation',
> > + "$ns [$field: $part_d_orig]"
> > + unless $ns eq "profile";
> > + tag 'invalid-restriction-label-in-source-relation',
> > + "$label [$field: $part_d_orig]"
> > + unless any { $_ eq $label } ("stage1", "notest");
>
>
> Two things here; one, Lintian emit
> "invalid-restriction-label-in-source-relation" when namespace is not
> "profile". This seems wrong to me. If Lintian does not know the
> namespace, Lintian should not claim to know the labels of said namespace.
Correct. Fixed.
> Secondly, I got a feeling that we want to move this to a data file (if
> not now, then eventually).
You probably mean somewhere in ./data/fields? But that would mean one file per
namespace, right?
> As mentioned earlier; we tend not to do the "conflict" tests. Feel free
> to skip them entirely.
Maybe there should be a function similar to implies() which takes depends as
well as conflicts into account?
> I think the "new_noarch" constructor might need to be updated to ignore
> these profiles as well. It is used to flag some missing
> build-dependencies in checks/rules.pm and checks/debhelper.pm (look for
> "_noarch"). Basically these checks ensures that a dependency is
> (probably) present on some architecture (and now also profile) when a
> given tool is used. Example:
>
> If you call dh_python, then there should be a dependency for the
> package providing dh_python. It may be architecture/profile dependent
> (e.g. not used during stage1), but some profile should need that
> dependency (else you wouldn't need to call dh_python).
Should I extend new_noarch to also strip profiles? In that case it should
probably also be renamed to match the new functionality? It seems that in all
cases where new_noarch is used right now it would also make sense to strip the
restriction list. So maybe new_noarch should become new_noarch_norestr or
new_norestriction?
> > + # Since the restriction list is not a set (as the architecture list) there
> > + # is no way to calculate a superset or subset of one another. Furthermore,
> > + # the evaluation depends on which build profiles are currently activated.
> > + # With n being the number of possible build profiles, 2^n checks would
> > + # have to be done. We decide not to do that (yet).
> > + return if defined $$p[6] or defined $$q[6];
> > +
>
> O(n^2) checking? Ugh. I suppose we could "normalise" the order before
> the comparison for slightly more accuracy, but still... ugh.
>
> Actually, if we (for a moment) ignore negative profiles (e.g. imagine a
> world without "!profile.stage1"), wouldn't this just be checking that
> the "larger" of the two is a subset of the other (assuming both are
> defined)?
How would this assumption help us?
Do you mean because in the special case that all restrictions are positive or
negative, the order of the restriction list does not matter anymore?
Yes, if a restriction list is all positive and all negative then order does not
matter and set comparisons as done with architectures are possible.
> Can you give me an example of where this O(n^2) kicks in?
Maybe I'm misunderstanding how that function is supposed to work. Whether or
not the restriction list influences dependency resolution currently depends
which profiles (if any) are currently activated. Lintian can't know which
profiles are activated at build time so (unless there exists a more clever
solution) it must check for all possible combinations of activated or
deactivated build profiles to be able to say something about dependency
implications.
The O(n^2) kicks in because of this:
Imagine that the two dependencies you want to compare only use one single
profile (lets say stage1), then the only two possibilities are
- no profile # column 2
- one profile (stage1) # column 3
(the column numbers as comments are for below table)
If there are two (lets say stage1 and notest) then the four possibilities are:
- no profile # column 2
- stage1 # column 3 with 2 options
- notest #
- stage1 notest # column 4
For the sake of the example with three possibilities (including stage2) that
is:
- no profile # column 2
- stage1 # column 3 with 3 options
- stage2
- notest
- stage1 stage2 # column 4 with 3 options
- stage2 notest
- stage1 notest
- stage1 stage2 notest # column 5
Lets make a table and lets have the first column indicate the number of
profiles, and the N-th column the number of possible combinations with N-1
profiles active. Let the last column indicate the sum of the second to second
to last column (the total amount of possible combinations for that amount of
possibly activated profiles).
1 1 1 - - - 2
2 1 2 1 - - 4
3 1 3 3 1 - 8
4 1 4 6 4 1 16
And we see pascal's triangle. The sum of the N-th row of pascal's triangle is
N^2 which corresponds to the last column above.
Though now that I look at the logic, there might be a simple way to test for
implication without checking N^2 possibilities. I'll look into it.
> Please add the new tags to the "Test-For" field in
> t/tests/fields-build-depends-general/desc
> (Please keep the field sorted by name of the tags)
Ah yes, I saw that file but forgot to make changes. Fixed now.
> If you have done it right, you should be able to run
> "private/update-coverage" and t/COVERAGE will *not* list any of your new
> tags. Feel free to omit the changes to t/COVERAGE though.
ooh, t/COVERAGE is a handy thing!
Thanks for helping me with this!
cheers, josch
From d331f1324f19613f317a1abadbb756f93db3fef5 Mon Sep 17 00:00:00 2001
From: josch <j.schauer@email.de>
Date: Mon, 3 Mar 2014 15:58:28 +0100
Subject: [PATCH] Build-profiles support
- added 3 new tags to detect errors in restriction list syntax
* invalid-restriction-term-in-source-relation
* invalid-restriction-namespace-in-source-relation
* invalid-restriction-label-in-source-relation
- added 4 new tags to ensure that if restrictions lists are used, a
versioned build dependency on dpkg-dev and (if applicable) debhelper
is added and no conflicts with them exist
* restriction-list-without-versioned-dpkg-dev-dependency
* restriction-list-with-versioned-dpkg-dev-conflict
* restriction-list-with-debhelper-without-debhelper-version
* restriction-list-with-debhelper-with-conflicting-debhelper-version
---
checks/fields.desc | 50 +++++++++++++++
checks/fields.pm | 71 +++++++++++++++++++---
lib/Lintian/Relation.pm | 18 +++++-
.../debian/debian/control.in | 7 ++-
t/tests/fields-build-depends-general/desc | 7 +++
t/tests/fields-build-depends-general/tags | 7 +++
6 files changed, 146 insertions(+), 14 deletions(-)
diff --git a/checks/fields.desc b/checks/fields.desc
index d55c11a..6061b95 100644
--- a/checks/fields.desc
+++ b/checks/fields.desc
@@ -637,6 +637,56 @@ Info: The architecture string in this source relation has some
negated. This is not permitted by Policy. Either all architectures must
be negated or none of them may be.
+Tag: invalid-restriction-term-in-source-relation
+Severity: important
+Certainty: certain
+Info: The restriction list in the source relation includes a term which
+ does not contain exactly one dot separating restriction namespace and label.
+ A term in a restriction list is of the form "namespace.label" (without the
+ quotes).
+
+Tag: invalid-restriction-namespace-in-source-relation
+Severity: important
+Certainty: possible
+Info: The restriction list in the source relation includes a term with
+ an unknown namespace. The only allowed namespace is "profile" (without the
+ quotes).
+
+Tag: invalid-restriction-label-in-source-relation
+Severity: important
+Certainty: possible
+Info: The restriction list in the source relation includes a term with
+ an unknown label. The only allowed labels are "stage1", "stage2", "notest"
+ and "cross".
+
+Tag: restriction-list-without-versioned-dpkg-dev-dependency
+Severity: normal
+Certainty: certain
+Info: If a restriction list appears in the build dependencies, then the
+ source package has to build depend on dpkg-dev (>= 1.17.2) for minimal
+ restriction list support.
+
+Tag: restriction-list-with-versioned-dpkg-dev-conflict
+Severity: normal
+Certainty: certain
+Info: If a restriction list appears in the build dependencies, then the
+ source package has to build depend on dpkg-dev (>= 1.17.2) for minimal
+ restriction list support. It must not conflict with version 1.17.2.
+
+Tag: restriction-list-with-debhelper-without-debhelper-version
+Severity: normal
+Certainty: certain
+Info: If a restriction list appears in the build dependencies and the
+ package uses debhelper, then the source package has to depend on at least
+ debhelper 9.20140227.
+
+Tag: restriction-list-with-debhelper-with-conflicting-debhelper-version
+Severity: normal
+Certainty: certain
+Info: If a restriction list appears in the build dependencies and the
+ package uses debhelper, then the source package has to depend on at least
+ debhelper 9.20140227. It must not conflict with version 9.20140227.
+
Tag: depends-on-build-essential-package-without-using-version
Severity: important
Certainty: certain
diff --git a/checks/fields.pm b/checks/fields.pm
index aeee252..0c76c96 100644
--- a/checks/fields.pm
+++ b/checks/fields.pm
@@ -730,10 +730,13 @@ sub run {
&& $known_obsolete_emacs{$alternatives[0]->[0]});
for my $part_d (@alternatives) {
- my ($d_pkg, $d_march, $d_version, $d_arch, $rest,
+ my ($d_pkg, $d_march, $d_version, $d_arch, $d_restr, $rest,
$part_d_orig)
= @$part_d;
+ # restriction lists do not appear in binary stanzas
+ $d_restr = undef;
+
tag 'versioned-provides', $part_d_orig
if ($field eq 'provides' && $d_version->[0]);
@@ -935,6 +938,7 @@ sub run {
any { $_ eq $_[0] } qw(build-depends build-depends-indep);
};
+ my $restrictions_used = 0;
my %depend;
for my $field (
qw(build-depends build-depends-indep build-conflicts build-conflicts-indep)
@@ -956,7 +960,7 @@ sub run {
&& &$is_dep_field($field));
for my $part_d (@alternatives) {
- my ($d_pkg, $d_march, $d_version, $d_arch, $rest,
+ my ($d_pkg, $d_march, $d_version, $d_arch, $d_restr, $rest,
$part_d_orig)
= @$part_d;
@@ -968,6 +972,31 @@ sub run {
}
}
+ if (scalar @{$d_restr} >= 1) {
+ $restrictions_used = 1;
+ }
+
+ for my $restr (@{$d_restr}) {
+ my $dotcount = $restr =~ tr/.//;
+ if ($dotcount != 1) {
+ tag 'invalid-restriction-term-in-source-relation',
+ "$restr [$field: $part_d_orig]";
+ next;
+ }
+ $restr =~ s/^!//;
+ my ($ns, $label) = split(/\./, $restr, 2);
+ if ($ns eq "profile") {
+ tag 'invalid-restriction-label-in-source-relation',
+ "$label [$field: $part_d_orig]"
+ unless any { $_ eq $label }
+ ("stage1", "stage2", "notest", "cross");
+ } else {
+ tag 'invalid-restriction-namespace-in-source-relation',
+ "$ns [$field: $part_d_orig]"
+ unless $ns eq "profile";
+ }
+ }
+
if ( $d_pkg =~ m/^openjdk-\d+-doc$/o
or $d_pkg eq 'classpath-doc'){
tag 'build-depends-on-specific-java-doc-package',
@@ -1077,6 +1106,24 @@ sub run {
}
}
+ # if restrictions are found in the build-depends/conflicts, then
+ # package must build-depend on dpkg (>= 1.17.2)
+ if ($restrictions_used) {
+ my $build_conflicts_all = $info->relation('build-conflicts-all');
+ tag 'restriction-list-without-versioned-dpkg-dev-dependency'
+ unless ($build_all->implies('dpkg-dev (>= 1.17.2)'));
+ tag 'restriction-list-with-versioned-dpkg-dev-conflict'
+ if ($build_conflicts_all->implies_inverse('dpkg-dev (<< 1.17.2)'));
+ # if the package uses debhelper then it must require and not
+ # conflict with version >= 9.20140227
+ if ($build_all->implies('debhelper')) {
+ tag 'restriction-list-with-debhelper-without-debhelper-version'
+ unless ($build_all->implies('debhelper (>= 9.20140227)'));
+ tag 'restriction-list-with-debhelper-with-conflicting-debhelper-version'
+ if ($build_conflicts_all->implies_inverse('debhelper (<< 9.20140227)'));
+ }
+ }
+
my (@arch_dep_pkgs, @dbg_pkgs);
foreach my $gproc ($group->get_binary_processables) {
my $binpkg = $gproc->pkg_name;
@@ -1238,16 +1285,16 @@ sub run {
return;
}
-# splits "foo:bar (>= 1.2.3) [!i386 ia64]" into
-# ( "foo", "bar", [ ">=", "1.2.3" ], [ [ "i386", "ia64" ], 1 ], "" )
-# ^^^ ^^
-# count of negated arches, if ! was given ||
-# rest (should always be "" for valid dependencies)
+# splits "foo:bar (>= 1.2.3) [!i386 ia64] <!profile.stage1 !profile.notest>" into
+# ( "foo", "bar", [ ">=", "1.2.3" ], [ [ "i386", "ia64" ], 1 ], [ "!profile.stage1" "!profile.notest" ], "" )
+# ^^^ ^^
+# count of negated arches, if ! was given ||
+# rest (should always be "" for valid dependencies)
sub _split_dep {
my $dep = shift;
- my ($pkg, $dmarch, $version, $darch) = ('', '', ['',''], [[], 0]);
+ my ($pkg, $dmarch, $version, $darch, $restr) = ('', '', ['',''], [[], 0], []);
- my $pkgname = $1 if $dep =~ s/^\s*([^\s\[\(]+)\s*//;
+ my $pkgname = $1 if $dep =~ s/^\s*([^<\s\[\(]+)\s*//;
if (defined $pkgname) {
($pkg, $dmarch) = split /:/, $pkgname, 2;
$dmarch //= ''; # Ensure it is defined (in case there is no ":")
@@ -1267,9 +1314,13 @@ sub _split_dep {
}
$darch->[1] = $negated;
}
+ if ($dep && $dep =~ s/\s*<([^>]+)>\s*//) {
+ my $t = $1;
+ $restr = [split /\s+/, $t];
+ }
}
- return ($pkg, $dmarch, $version, $darch, $dep);
+ return ($pkg, $dmarch, $version, $darch, $restr, $dep);
}
sub perl_core_has_version {
diff --git a/lib/Lintian/Relation.pm b/lib/Lintian/Relation.pm
index 1ba783c..4ec8238 100644
--- a/lib/Lintian/Relation.pm
+++ b/lib/Lintian/Relation.pm
@@ -115,21 +115,26 @@ sub parse_element {
\s* (.*?) # architectures (5)
\s* \] # closing bracket
)? # end of optional architecture
+ (?: # start of optional restriction
+ \s* < # open bracket for restriction
+ \s* (.*?) # don't parse restrictions now
+ \s* > # closing bracket
+ )? # end of optional restriction
/x;
- my ($pkgname, $march, $relop, $relver, $bdarch) = ($1, $2, $3, $4, $5);
+ my ($pkgname, $march, $relop, $relver, $bdarch, $restr) = ($1, $2, $3, $4, $5, $6);
my @array;
if (not defined($relop)) {
# If there's no version, we don't need to do any further processing.
# Otherwise, convert the legacy < and > relations to the current ones.
- @array = ('PRED', $pkgname, undef, undef, $bdarch, $march);
+ @array = ('PRED', $pkgname, undef, undef, $bdarch, $march, $restr);
} else {
if ($relop eq '<') {
$relop = '<<';
} elsif ($relop eq '>') {
$relop = '>>';
}
- @array = ('PRED', $pkgname, $relop, $relver, $bdarch, $march);
+ @array = ('PRED', $pkgname, $relop, $relver, $bdarch, $march, $restr);
}
# Optimise the memory usage of the array. Understanding this
@@ -351,6 +356,13 @@ sub implies_element {
$$q[1] = '' unless defined $$q[1];
return if $$p[1] ne $$q[1];
+ # Since the restriction list is not a set (as the architecture list) there
+ # is no way to calculate a superset or subset of one another. Furthermore,
+ # the evaluation depends on which build profiles are currently activated.
+ # With n being the number of possible build profiles, 2^n checks would
+ # have to be done. We decide not to do that (yet).
+ return if defined $$p[6] or defined $$q[6];
+
# If the names match, then the only difference is in the architecture or
# version clauses. First, check architecture. The architectures for p
# must be a superset of the architectures for q.
diff --git a/t/tests/fields-build-depends-general/debian/debian/control.in b/t/tests/fields-build-depends-general/debian/debian/control.in
index f6546b9..80222ef 100644
--- a/t/tests/fields-build-depends-general/debian/debian/control.in
+++ b/t/tests/fields-build-depends-general/debian/debian/control.in
@@ -8,8 +8,13 @@ Build-Depends: debhelper (>= 9), bd-conflict, revision-1 (>= 1.0-1),
xorg-dev, java-propose-classpath, python3.2-dev, foo [all],
bar [i386 any], baz [source i3!86], baz [i386 !amd64],
other-pkg [kfreebsd-any], yet-another [any-powerpc],
+ big <profile.stage1>, bpfail1 <foobar>,
+ bpfail2 <foo.bar>, bpfail3 <profile.bar>,
packaging-dev, libdb5.1++-dev, libdb5.1-java-dev
-Build-Conflicts: bd-conflict
+Build-Conflicts:
+ bd-conflict,
+ dpkg-dev (>= 1.17.2),
+ debhelper (>= 9.20140227)
Package: {$source}
Architecture: {$architecture}
diff --git a/t/tests/fields-build-depends-general/desc b/t/tests/fields-build-depends-general/desc
index b8b7a1b..712ebd7 100644
--- a/t/tests/fields-build-depends-general/desc
+++ b/t/tests/fields-build-depends-general/desc
@@ -16,5 +16,12 @@ Test-For:
depends-on-build-essential-package-without-using-version
depends-on-packaging-dev
invalid-arch-string-in-source-relation
+ invalid-restriction-label-in-source-relation
+ invalid-restriction-namespace-in-source-relation
+ invalid-restriction-term-in-source-relation
ored-build-depends-on-obsolete-package
+ restriction-list-with-debhelper-with-conflicting-debhelper-version
+ restriction-list-with-debhelper-without-debhelper-version
+ restriction-list-with-versioned-dpkg-dev-conflict
+ restriction-list-without-versioned-dpkg-dev-dependency
References: Debian Bug#540594, Debian Bug#551793
diff --git a/t/tests/fields-build-depends-general/tags b/t/tests/fields-build-depends-general/tags
index 0c0222f..1bfb5eb 100644
--- a/t/tests/fields-build-depends-general/tags
+++ b/t/tests/fields-build-depends-general/tags
@@ -9,9 +9,16 @@ E: fields-build-depends-general source: depends-on-build-essential-package-witho
E: fields-build-depends-general source: invalid-arch-string-in-source-relation all [build-depends: foo [all]]
E: fields-build-depends-general source: invalid-arch-string-in-source-relation i3!86 [build-depends: baz [source i3!86]]
E: fields-build-depends-general source: invalid-arch-string-in-source-relation source [build-depends: baz [source i3!86]]
+E: fields-build-depends-general source: invalid-restriction-label-in-source-relation bar [build-depends: bpfail3 <profile.bar>]
+E: fields-build-depends-general source: invalid-restriction-namespace-in-source-relation foo [build-depends: bpfail2 <foo.bar>]
+E: fields-build-depends-general source: invalid-restriction-term-in-source-relation foobar [build-depends: bpfail1 <foobar>]
I: fields-build-depends-general source: build-depends-on-python-dev-with-no-arch-any
I: fields-build-depends-general source: ored-build-depends-on-obsolete-package build-depends: xlibmesa-gl-dev
W: fields-build-depends-general source: build-depends-on-1-revision build-depends: revision-1 (>= 1.0-1)
W: fields-build-depends-general source: build-depends-on-versioned-berkeley-db build-depends:libdb5.1++-dev
W: fields-build-depends-general source: build-depends-on-versioned-berkeley-db build-depends:libdb5.1-java-dev
W: fields-build-depends-general source: depends-on-packaging-dev build-depends
+W: fields-build-depends-general source: restriction-list-with-debhelper-with-conflicting-debhelper-version
+W: fields-build-depends-general source: restriction-list-with-debhelper-without-debhelper-version
+W: fields-build-depends-general source: restriction-list-with-versioned-dpkg-dev-conflict
+W: fields-build-depends-general source: restriction-list-without-versioned-dpkg-dev-dependency
--
1.8.5.3
Reply to: