Bug#740607: lintian: Please support build-profiles
On 2014-03-03 16:09, Johannes Schauer wrote:
> Package: lintian
> Version: 2.5.22
> Severity: wishlist
> Tags: patch
>
> Hi,
>
> I implemented a preliminary patch for lintian to understand and check
> the restriction syntax described in this document:
> https://wiki.debian.org/BuildProfileSpec
>
Hi,
Thanks for looking into this.
> Please tell me what needs to be changed so that I can get this patch
> into an acceptable shape and excuse my weak perl-foo :)
>
Certainly; find my comments interleaved in your patch below.
> Depending on the outcome of this thread [1] an amendment might be
> necessary in the future to allow a new field in DEBIAN/control.
>
> cheers, josch
>
Noted; generally that is just modifying (a subset of the) data files
matching data/{fields,common}/*-fields.
> [1] http://lists.debian.org/20140221113013.18244.3579@hoothoot
>
>
> 0001-Build-profiles-support.patch
>
>
>>From 33a76313fc1ecb5d7fd970c5ed41b9da12e85269 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 | 64 ++++++++++++++++++----
> lib/Lintian/Relation.pm | 18 +++++-
> .../debian/debian/control.in | 6 +-
> t/tests/fields-build-depends-general/tags | 7 +++
> 5 files changed, 131 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-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"
> [...]
> +
> +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-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.
> +
Usually, we don't bother doing "conflict" testing. At least, we skip it
for regular debhelper dependencies and many other cases.
> 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..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).
> @@ -935,6 +935,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 +957,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 +969,27 @@ sub run {
> }
> }
>
> + if (scalar @{$d_restr} >= 1) {
> + $restrictions_used = 1;
> + }
> +
> + for my $restr (@{$d_restr}) {
> + my $dotcount = () = $restr =~ /\./g;
You probably want:
my $dotcount = $restr =~ tr/.//;
Alternatively:
my $dotcount;
$dotcount++ while ($dotcount =~ m/\./g);
(Note that the tr does /not/ need escaping of the period.)
> + if ($dotcount != 1) {
> + tag 'invalid-restriction-term-in-source-relation',
> + "$restr [$field: $part_d_orig]";
> + next;
> + }
> + $restr =~ s/^!//;
> + 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.
Secondly, I got a feeling that we want to move this to a data file (if
not now, then eventually).
> + }
> +
> if ( $d_pkg =~ m/^openjdk-\d+-doc$/o
> or $d_pkg eq 'classpath-doc'){
> tag 'build-depends-on-specific-java-doc-package',
> @@ -1077,6 +1099,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)'));
> + }
> + }
> +
As mentioned earlier; we tend not to do the "conflict" tests. Feel free
to skip them entirely.
> my (@arch_dep_pkgs, @dbg_pkgs);
> foreach my $gproc ($group->get_binary_processables) {
> my $binpkg = $gproc->pkg_name;
> @@ -1238,16 +1278,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 +1307,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
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).
> [...]
>
> # 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];
> +
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)?
Can you give me an example of where this O(n^2) kicks in?
> # 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..12acd64 100644
> --- a/t/tests/fields-build-depends-general/debian/debian/control.in
> +++ b/t/tests/fields-build-depends-general/debian/debian/control.in
> [...]
> diff --git a/t/tests/fields-build-depends-general/tags b/t/tests/fields-build-depends-general/tags
> index 0c0222f..c5413f0 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: bpfail2 <foo.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
>
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)
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.
Once again, thanks for looking into this.
~Niels
Reply to: