Le dimanche 16 août 2015, 14:20:33 Niels Thykier a écrit :
> Control: tags -1 - moreinfo
>
> On 2015-08-16 00:13, Rafael Kitover wrote:
> > Package: lintian
> > Version: 2.5.35
> > Severity: normal
> >
> > This patch fixes lack-of-common-license-reference false positives in
> > new-style copyright files when a license refers to one of these common
> > licenses.
> >
> > It includes a test for this against all common licenses.
> >
> > All testsuite tests pass.
> >
> > The patch is against the debcheckout of lintian.
> >
> > [...]
> >
>
> Hi,
>
> Thanks for looking in to this.
>
> I do have some concerns with the implementation, see below interleaved.
>
> > 0001-fix-common-lic.-false-pos.-in-new-style-copyright.patch
> >
> >
> >>From 441f44c5be0fe70d9a86b3ee0cec49430b0c2a9d Mon Sep 17 00:00:00 2001
> > From: Rafael Kitover <rkitover@gmail.com>
> > Date: Sat, 15 Aug 2015 17:50:34 -0400
> > Subject: [PATCH] fix common lic. false pos. in new-style copyright
> >
> > Fix false positives for lack of common license references in new-style
> > copyright files when a license refers to another license, by trying to
> > parse the file and then checking both the names of the licenses and the
> > texts.
> >
> > Add new test for references to common licenses as well.
> >
> > The test suite passes with these changes.
> > ---
> > [...]
> >
> > diff --git a/checks/copyright-file.pm b/checks/copyright-file.pm
> > index c6e35ef..09b664e 100644
> > --- a/checks/copyright-file.pm
> > +++ b/checks/copyright-file.pm
> > [...]
> > @@ -373,6 +426,45 @@ sub check_cross_link {
> > return;
> > }
> >
> > +# Checks the name and text of every license in the file against given name and
> > +# text check coderefs, if the file is in the new format, if the file is in the
> > +# old format only runs the text coderef against the whole file.
> > +sub check_names_texts {
>
> This is called multiple times. Each time it opens and parses the
> copyright file at least once each.
>
> I would like to see this done smarter and less wastefully. Please note
> that we have some very large copyright files or/and source packages
> producing many binaries with a copyright file in each of them.
>
> Have you looked at the performance of this check on some packages?
> Lintian has a few built-in options to help you here:
I would not try to parse anything except copyright 1.0 format.
The other format is slowly dying...
>
> $ lintian --help=extended | grep perf
> --perf-debug turn on performance debugging
> --perf-output X send performance logging to file (...)
>
>
> > + my ($name_check, $text_check, $file) = @_;
> > +
> > + local $@;
> > + eval {
> > + foreach my $paragraph (read_dpkg_control($file)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^
If you use copyright 1.0 we have parsed the format and we have hash
> (First read of the file.)
>
> > + next
> > + unless (keys %$paragraph == 1)
> > + && ((keys %$paragraph)[0] =~ /^license$/i);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Please use "exists($paragraph->{'license'})" instead.
Yes
>
> > +
> > + my ($license_name, $license_text)
> > + = (values %$paragraph)[0] =~ /^([^\r\n]+)\r?\n(.*)\z/s;
> > +
> > + my $matches = do {
> > + local $_ = $license_name || '';
> > + $name_check->($_);
>
> $name_check is always a single regex. Consider passing it as a qr// and
> do a normal regex check here instead of calling a separate sub. This
> would also avoid the "local $_", which is somewhat expensive.
>
> > + }
> > + && do {
> > + local $_ = $license_text || '';
> > + $text_check->($_);
> > + };
>
> If we could avoid the subroutine + local $_; here too that would be nice
> as well.
>
> > +
> > + die 'MATCH' if $matches;
> > + }
> > + };
> > + if ($@)
> > + { # match or parse error: copyright not in new format, just check text
> > + return 1 if $@ =~ /^MATCH/;
> > +
> > + local $_ = slurp_entire_file($file);
> ^^^^^^^^^^^^^^^^^^^^^^^^
> (Second read of the file.)
>
> > + return $text_check->($_);
> > + }
> > +
> > + return; # did not match anything
> > +}
> > +
> > 1;
> >
> > # Local Variables:
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/control.in b/t/tests/copyright-file-non-common-license/debian/debian/control.in
> > new file mode 100644
> > index 0000000..e80822d
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/control.in
> > @@ -0,0 +1,94 @@
> > +Source: {$source}
> > +Priority: extra
> > +Section: {$section}
> > +Maintainer: {$author}
> > +Standards-Version: {$standards_version}
> > +Build-Depends: debhelper (>= 9)
> > +
> > +Package: copyright-mentions-apache
> > +Architecture: all
> > +Depends: $\{shlibs:Depends\}, $\{misc:Depends\}
> > +Description: checks against common license false positive for Apache
> > + Tests against common license false positive for Apache.
> > + .
> > + This is a test package designed to exercise some feature or tag of
> > + Lintian. It is part of the Lintian test suite and may do very odd
> > + things. It should not be installed like a regular package. It may
> > + be an empty package.
> > +
> > +Package: copyright-mentions-apache2
> > +Architecture: all
> > +Depends: $\{shlibs:Depends\}, $\{misc:Depends\}
> > +Description: checks against common license false positive for Apache (2)
> > + Tests against common license false positive for Apache (2).
> > + .
> > + This is a test package designed to exercise some feature or tag of
> > + Lintian. It is part of the Lintian test suite and may do very odd
> > + things. It should not be installed like a regular package. It may
> > + be an empty package.
> > +
> > +Package: copyright-mentions-apache3
> > +Architecture: all
> > +Depends: $\{shlibs:Depends\}, $\{misc:Depends\}
> > +Description: checks against common license false positive for Apache (3)
> > + Tests against common license false positive for Apache (3).
> > + .
> > + This is a test package designed to exercise some feature or tag of
> > + Lintian. It is part of the Lintian test suite and may do very odd
> > + things. It should not be installed like a regular package. It may
> > + be an empty package.
> > +
> > +Package: copyright-mentions-gfdl
> > +Architecture: all
> > +Depends: $\{shlibs:Depends\}, $\{misc:Depends\}
> > +Description: checks against common license false positive for gfdl
> > + Tests against common license false positive for gfdl.
> > + .
> > + This is a test package designed to exercise some feature or tag of
> > + Lintian. It is part of the Lintian test suite and may do very odd
> > + things. It should not be installed like a regular package. It may
> > + be an empty package.
> > +
> > +Package: copyright-mentions-gpl
> > +Architecture: all
> > +Depends: $\{shlibs:Depends\}, $\{misc:Depends\}
> > +Description: checks against common license false positive for gpl
> > + Tests against common license false positive for gpl.
> > + .
> > + This is a test package designed to exercise some feature or tag of
> > + Lintian. It is part of the Lintian test suite and may do very odd
> > + things. It should not be installed like a regular package. It may
> > + be an empty package.
> > +
> > +Package: copyright-mentions-lgpl
> > +Architecture: all
> > +Depends: $\{shlibs:Depends\}, $\{misc:Depends\}
> > +Description: checks against common license false positive for lgpl
> > + Tests against common license false positive for lgpl.
> > + .
> > + This is a test package designed to exercise some feature or tag of
> > + Lintian. It is part of the Lintian test suite and may do very odd
> > + things. It should not be installed like a regular package. It may
> > + be an empty package.
> > +
> > +Package: copyright-mentions-lgpl2
> > +Architecture: all
> > +Depends: $\{shlibs:Depends\}, $\{misc:Depends\}
> > +Description: checks against common license false positive for lgpl (2)
> > + Tests against common license false positive for lgpl (2).
> > + .
> > + This is a test package designed to exercise some feature or tag of
> > + Lintian. It is part of the Lintian test suite and may do very odd
> > + things. It should not be installed like a regular package. It may
> > + be an empty package.
> > +
> > +Package: copyright-mentions-perl
> > +Architecture: all
> > +Depends: $\{shlibs:Depends\}, $\{misc:Depends\}
> > +Description: checks against common license false positive for perl
> > + Tests against common license false positive for perl.
> > + .
> > + This is a test package designed to exercise some feature or tag of
> > + Lintian. It is part of the Lintian test suite and may do very odd
> > + things. It should not be installed like a regular package. It may
> > + be an empty package.
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache.copyright b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache.copyright
> > new file mode 100644
> > index 0000000..b4fe499
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache.copyright
> > @@ -0,0 +1,13 @@
> > +Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> > +Upstream-Name: lintian
> > +Upstream-Contact: Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +Source: http://git.debian.org/?p=lintian/lintian.git
> > +
> > +Files: *
> > +Copyright: 2015 Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +License: Mentions-Other-License
> > +
> > +License: Mentions-Other-License
> > + lorem ipsum
> > + lorem ipsum Apache License , Version 2.0 lorem ipsum
> > + lorem ipsum
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache2.copyright b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache2.copyright
> > new file mode 100644
> > index 0000000..035ee22
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache2.copyright
> > @@ -0,0 +1,13 @@
> > +Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> > +Upstream-Name: lintian
> > +Upstream-Contact: Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +Source: http://git.debian.org/?p=lintian/lintian.git
> > +
> > +Files: *
> > +Copyright: 2015 Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +License: Mentions-Other-License
> > +
> > +License: Mentions-Other-License
> > + lorem ipsum
> > + lorem ipsum Apache License Version 2.0 lorem ipsum
> > + lorem ipsum
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache3.copyright b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache3.copyright
> > new file mode 100644
> > index 0000000..dab0d47
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-apache3.copyright
> > @@ -0,0 +1,13 @@
> > +Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> > +Upstream-Name: lintian
> > +Upstream-Contact: Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +Source: http://git.debian.org/?p=lintian/lintian.git
> > +
> > +Files: *
> > +Copyright: 2015 Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +License: Mentions-Other-License
> > +
> > +License: Mentions-Other-License
> > + lorem ipsum
> > + lorem ipsum Apache-2 License lorem ipsum
> > + lorem ipsum
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-gfdl.copyright b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-gfdl.copyright
> > new file mode 100644
> > index 0000000..5a8f46b
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-gfdl.copyright
> > @@ -0,0 +1,13 @@
> > +Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> > +Upstream-Name: lintian
> > +Upstream-Contact: Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +Source: http://git.debian.org/?p=lintian/lintian.git
> > +
> > +Files: *
> > +Copyright: 2015 Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +License: Mentions-Other-License
> > +
> > +License: Mentions-Other-License
> > + lorem ipsum
> > + lorem ipsum GNU Free Documentation License (GFDL) lorem ipsum
> > + lorem ipsum
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-gpl.copyright b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-gpl.copyright
> > new file mode 100644
> > index 0000000..248debf
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-gpl.copyright
> > @@ -0,0 +1,14 @@
> > +Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> > +Upstream-Name: lintian
> > +Upstream-Contact: Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +Source: http://git.debian.org/?p=lintian/lintian.git
> > +
> > +Files: *
> > +Copyright: 2015 Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +License: Mentions-Other-License
> > +
> > +License: Mentions-Other-License
> > + lorem ipsum
> > + lorem ipsum GNU General Public License (GPL) applies to the changes,
> > + .
> > + lorem ipsum
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-lgpl.copyright b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-lgpl.copyright
> > new file mode 100644
> > index 0000000..89c5e79
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-lgpl.copyright
> > @@ -0,0 +1,13 @@
> > +Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> > +Upstream-Name: lintian
> > +Upstream-Contact: Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +Source: http://git.debian.org/?p=lintian/lintian.git
> > +
> > +Files: *
> > +Copyright: 2015 Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +License: Mentions-Other-License
> > +
> > +License: Mentions-Other-License
> > + lorem ipsum
> > + lorem ipsum GNU Lesser General Public License (LGPL) lorem ipsum
> > + lorem ipsum
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-lgpl2.copyright b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-lgpl2.copyright
> > new file mode 100644
> > index 0000000..291c0e6
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-lgpl2.copyright
> > @@ -0,0 +1,13 @@
> > +Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> > +Upstream-Name: lintian
> > +Upstream-Contact: Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +Source: http://git.debian.org/?p=lintian/lintian.git
> > +
> > +Files: *
> > +Copyright: 2015 Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +License: Mentions-Other-License
> > +
> > +License: Mentions-Other-License
> > + lorem ipsum
> > + lorem ipsum GNU Library General Public License (LGPL) lorem ipsum
> > + lorem ipsum
> > diff --git a/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-perl.copyright b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-perl.copyright
> > new file mode 100644
> > index 0000000..b2c896a
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/debian/debian/copyright-mentions-perl.copyright
> > @@ -0,0 +1,13 @@
> > +Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> > +Upstream-Name: lintian
> > +Upstream-Contact: Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +Source: http://git.debian.org/?p=lintian/lintian.git
> > +
> > +Files: *
> > +Copyright: 2015 Lintian Maintainers <debian-lint-maint@lists.debian.org>
> > +License: Mentions-Other-License
> > +
> > +License: Mentions-Other-License
> > + lorem ipsum
> > + lorem ipsum under the same terms as Perl itself lorem ipsum
> > + lorem ipsum
> > diff --git a/t/tests/copyright-file-non-common-license/desc b/t/tests/copyright-file-non-common-license/desc
> > new file mode 100644
> > index 0000000..c652192
> > --- /dev/null
> > +++ b/t/tests/copyright-file-non-common-license/desc
> > @@ -0,0 +1,12 @@
> > +Testname: copyright-file-non-common-license
> > +Sequence: 6000
> > +Version: 1.0
> > +Description: Test for false positive for a common license
> > +Skeleton: pedantic
> > +Options: -IE --pedantic
> > +Test-Against:
> > + copyright-should-refer-to-common-license-file-for-gpl
> > + copyright-should-refer-to-common-license-file-for-gfdl
> > + copyright-should-refer-to-common-license-file-for-lgpl
> > + copyright-should-refer-to-common-license-file-for-apache-2
> > + copyright-file-lacks-pointer-to-perl-license
> > diff --git a/t/tests/copyright-file-non-common-license/tags b/t/tests/copyright-file-non-common-license/tags
> > new file mode 100644
> > index 0000000..e69de29
> > -- 2.1.4
> >
>
Attachment:
signature.asc
Description: This is a digitally signed message part.