Bug#790323: lintian: warn when init.d script for rcS does not have a native systemd unit

[Niels Thykier <niels@thykier.net>]

On 2015-06-28 03:09, Felipe Sateler wrote:
> Package: lintian
> Version: 2.5.32
> Severity: wishlist
> Tags: patch
> 
> Hi,
> 
> Please find attached a patch that does $subject. I have taken the
> liberty to refactor the code a bit in order to stop tagging multiple
> times for the same error.
> 
> Patches 1-3 are the refactoring, patch 4 is the new check. There is a
> test for the new check.
> 

Hi,

Thanks for working on this.  I have a couple of remarks to some of the
patches (interleaved into the patches below).

> I'm wondering if tag systemd-no-service-for-init-script should be
> lowered in severity but added inconditionally... but that is a separate
> issue. If/when this patch is merged, I can provide a patch for changing
> that so we can discuss that.
> 

Ok. :)

> [...]
> 
> 0001-Reorder-systemd-checks.patch
> 
> 
>>From 1c4ad47fead2a6d32b5fdc6888ba7b5333804bcb Mon Sep 17 00:00:00 2001
> From: Felipe Sateler <fsateler@debian.org>
> Date: Sat, 27 Jun 2015 16:32:42 -0300
> Subject: [PATCH 1/4] Reorder systemd checks
> 
> This reorder groups most checks inside the corresponding check_*
> ---
>  checks/systemd.pm | 140 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 79 insertions(+), 61 deletions(-)
> 
> diff --git a/checks/systemd.pm b/checks/systemd.pm
> index d36cf65..4a45b49 100644
> --- a/checks/systemd.pm
> +++ b/checks/systemd.pm
> [...]
> +sub get_systemd_service_files {
> +    my $info = shift @_;

Please use the "my ($info) = @_;" notation instead.

The use of "shift" is generally discouraged throughout the lintian code
base (I think we

> +    
> +    return grep { m,/systemd/system/.*\.service$, } $info->sorted_index;
> +}
> +
> +sub get_systemd_service_names {
> +    my ($info) = @_;
> +    my %services;
> +    
> +    [...]
> +    return %services;
> +}

Please consider returning this as a ref.  Since it is passed around, we
end up copying it several times.  Admittedly, I suspect it is a small
hash, I am mostly in it for being consistent with similar usage for
larger hashes.

Quick cheat-sheet

  return \%services;
  $services{foo} => $services->{foo}
  if (%foo) => if (%{$foo})

> +
>  sub check_systemd_service_file {
>      my ($info, $file) = @_;
>  
> +    tag 'systemd-service-file-outside-lib', $file if ($file =~ m,^etc/systemd/system/,);
> +    tag 'systemd-service-file-outside-lib', $file if ($file =~ m,^usr/lib/systemd/system/,);

Note this will match non-files (including the etc/systemd/system
directory).  If the systemd package ships that as an empty dir (or
containing a README).

Though presumably something already filters this out before we get that far?

> +    

An "empty" whitespace line - please apply "perltidy -it=4 -b
checks/systemd.pm" (it will possibly reformat other things as well).

>      my @values = extract_service_file_values($info, $file, 'Unit', 'After');
>      my @obsolete = grep { /^(?:syslog|dbus)\.target$/ } @values;
>      tag 'systemd-service-file-refers-to-obsolete-target', $file, $_
> @@ -236,13 +261,6 @@ sub extract_service_file_values {
> [...]
> 
> 0002-Check-files-as-we-detect-them-and-discard-invalid-fi.patch
> 
> 
>>From f712f9246412799088f2893cb5323b8b9f295de3 Mon Sep 17 00:00:00 2001
> From: Felipe Sateler <fsateler@debian.org>
> Date: Sat, 27 Jun 2015 21:56:11 -0300
> Subject: [PATCH 2/4] Check files as we detect them, and discard invalid files
> 
> prevents duplicate service-file-is-not-a-file

\o/

> ---
>  checks/systemd.pm            | 32 +++++++++++++++++---------------
>  t/tests/systemd-general/tags |  1 -
>  2 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/checks/systemd.pm b/checks/systemd.pm
> index 4a45b49..2fd2c82 100644
> --- a/checks/systemd.pm
> +++ b/checks/systemd.pm
> [...]
> @@ -124,12 +120,18 @@ sub check_init_script {
> [...]
>  sub get_systemd_service_names {
> -    my ($info) = @_;
> +    my $info = shift @_;
> +    my @files = @_;

Again, please prefer:

  my ($info, @files) = @_;

Alternatively, consider making @files a ref.  Again, not a high priority
since I doubt any package will ever ship a "significant" amount of
service files.

If you do, the cheat sheet is:

  get_systemd_service_names($info, @files) =>
     get_systemd_service_names($info, \@files)
  my ($info, $files_ref) = @_;
  @files => @{$files_ref}

>      my %services;
>      
>      my $safe_add_service = sub {
> @@ -141,7 +143,7 @@ sub get_systemd_service_names {
> [...]

>>From f98b16ffd7c8adb603fa6de4afc9dfc06c142764 Mon Sep 17 00:00:00 2001
> From: Felipe Sateler <fsateler@debian.org>
> Date: Sat, 27 Jun 2015 22:01:19 -0300
> Subject: [PATCH 3/4] Add parameter to prevent tagging when parsing values
> 
> Enables us to prevent multiple service-key-has-whitespace
> [...]

\o/