Merge ~michal-maloszewski99/ubuntu/+source/logwatch:logwatch-lp-1890748-focal into ubuntu/+source/logwatch:ubuntu/focal-devel

[Michał Małoszewski]

PPA: https://launchpad.net/~michal-maloszewski99/+archive/ubuntu/lp-1890748-logwatch-focal-ppa

[Robie Basak (racb)]

This looks good - it looks like it correctly fixes the problem. A few things need fixing though please:

The debian/patches/ directory in this branch contains both focal-logwatch-ifconfig-fix.patch and logwatch-focal-lp1890748-fix.patch. It's not really a problem to rename the patch, but please don't keep the unused patch around. It might not be worth the noise to even rename it even if the new name is an improvement though - keep in mind that the previous failed fix is on the permanent record now.

logwatch 7.5.2-1ubuntu1.4 is now published in focal-proposed so that version number can't be used again. Please add a new changelog entry instead of adjusting the previous one, and explain what you're fixing. It will have to be version 7.5.2-1ubuntu1.5.

In the git commit message, please avoid a title of "Fix the logwatch package". This is implied - of course an SRU to the logwatch package is to fix the logwatch package. So it basically doesn't convey any information. Instead, how about something like "Correctly fall back to ifconfig"?

review: Needs Fixing

[Michał Małoszewski (michal-maloszewski99)]

Thanks for review, that makes sense. I will make some changes.

[Utkarsh Gupta (utkarsh)]

No "Origin" field and "Forwarded" is no? This doesn't make sense - there should be something, at least. Where is that patch from?

review: Needs Fixing

[Michał Małoszewski (michal-maloszewski99)]

Utkarsh, this is change which was coded by hand.
Forwaded is no, but below (was, because I messed up something but I will correct it asap)
X-Not-Forwarded-Reason: The change is only needed for focal - which explains the reason of set 'Forwarded' to 'no'.

[Michał Małoszewski (michal-maloszewski99)]

Update with PPA

Results: (from http://autopkgtest.ubuntu.com/results/autopkgtest-focal-michal-maloszewski99-lp-1890748-logwatch-focal-ppa/?format=plain)
  logwatch @ amd64:
    http://autopkgtest.ubuntu.com/results/autopkgtest-focal-michal-maloszewski99-lp-1890748-logwatch-focal-ppa/focal/amd64/l/logwatch/20230421_124251_3aacb@/log.gz
    21.04.23 12:42:51 ✅ Triggers: logwatch/7.5.2-1ubuntu1.5~ppa2
  logwatch @ arm64:
    http://autopkgtest.ubuntu.com/results/autopkgtest-focal-michal-maloszewski99-lp-1890748-logwatch-focal-ppa/focal/arm64/l/logwatch/20230421_124544_87d71@/log.gz
    21.04.23 12:45:44 ✅ Triggers: logwatch/7.5.2-1ubuntu1.5~ppa2
  logwatch @ armhf:
    http://autopkgtest.ubuntu.com/results/autopkgtest-focal-michal-maloszewski99-lp-1890748-logwatch-focal-ppa/focal/armhf/l/logwatch/20230421_124201_0a988@/log.gz
    21.04.23 12:42:01 ✅ Triggers: logwatch/7.5.2-1ubuntu1.5~ppa2
  logwatch @ ppc64el:
    http://autopkgtest.ubuntu.com/results/autopkgtest-focal-michal-maloszewski99-lp-1890748-logwatch-focal-ppa/focal/ppc64el/l/logwatch/20230421_124509_dee79@/log.gz
    21.04.23 12:45:09 ✅ Triggers: logwatch/7.5.2-1ubuntu1.5~ppa2
Running: (none)
Waiting: (none)

Ready to be re-reviewed.

[Michał Małoszewski (michal-maloszewski99)]

fyi - sru template is the one that is attached to the bug report

[Bryce Harrington (bryce)]

LGTM. Ran through the test case with and without the PPA, and worked as expected. I'll sponsor the upload.

A couple notes just for general education, but they're not important enough for fixing here:

Watch out for whitespace changes in your patches, especially in SRUs, because they have no functional change. You want the diff to really highlight the specific line(s) necessary for the change, and whitespace delta thus just adds noise, so reviewers have to doublecheck things. In this case it's fine, since this is part of the cleanup of the fix that failed validation anyway. (I do note though the tabbing still seems off, but honestly the entire zz-network script has messy whitespace and tabbing already...)

I notice the patch is moved to the end of debian/series, which I gather is just for cleanup. But be aware in a proper SRU if you move where the patch is listed, it should probably also be noted in the debian/changelog, and should really only be done if it is needed to fix an actual problem. In this case, since it's just a cleanup of the earlier upload it's fine.

The output from running the test case is:

 ------------- Network statistics ---------------

 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
     inet 127.0.0.1/8 scope host lo
        valid_lft forever preferred_lft forever
     inet6 ::1/128 scope host
        valid_lft forever preferred_lft forever
 56: eth0@if57: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
     link/ether 00:16:3e:a6:2e:24 brd ff:ff:ff:ff:ff:ff link-netnsid 0
     inet 10.69.244.129/24 brd 10.69.244.255 scope global dynamic eth0
        valid_lft 3421sec preferred_lft 3421sec
     inet6 fd42:7d02:9a49:5192:216:3eff:fea6:2e24/64 scope global mngtmpaddr noprefixroute
        valid_lft forever preferred_lft forever
     inet6 fe80::216:3eff:fea6:2e24/64 scope link
        valid_lft forever preferred_lft forever

 sh: 1: netstat: not found

 ------------- Network statistics ---------------

The error about netstat is unrelated to this fix; it comes from a subsequent system() call that the ifconfig/ip issue was hiding.

review: Approve

[Bryce Harrington (bryce)]

$ grep ^Vcs ../logwatch_7.5.2-1ubuntu1.5_source.changes
Vcs-Git: https://git.launchpad.net/~bryce/ubuntu/+source/logwatch
Vcs-Git-Commit: e00b7ade4a7b342b6137b1e4946051dd5dd0d38a
Vcs-Git-Ref: refs/heads/logwatch-lp-1890748-focal

$ debsponsor ../logwatch_7.5.2-1ubuntu1.5_source.changes
 signfile dsc ../logwatch_7.5.2-1ubuntu1.5.dsc A661100B3DAC1D4F2CAD8A54E603B2578FB8F0FB

 fixup_buildinfo ../logwatch_7.5.2-1ubuntu1.5.dsc ../logwatch_7.5.2-1ubuntu1.5_source.buildinfo
 signfile buildinfo ../logwatch_7.5.2-1ubuntu1.5_source.buildinfo A661100B3DAC1D4F2CAD8A54E603B2578FB8F0FB

 fixup_changes dsc ../logwatch_7.5.2-1ubuntu1.5.dsc ../logwatch_7.5.2-1ubuntu1.5_source.changes
 fixup_changes buildinfo ../logwatch_7.5.2-1ubuntu1.5_source.buildinfo ../logwatch_7.5.2-1ubuntu1.5_source.changes
 signfile changes ../logwatch_7.5.2-1ubuntu1.5_source.changes A661100B3DAC1D4F2CAD8A54E603B2578FB8F0FB

Successfully signed dsc, buildinfo, changes files

$ gpg --verify ../logwatch_7.5.2-1ubuntu1.5_source.changes
gpg: Signature made Thu 27 Apr 2023 11:11:51 AM PDT
gpg: using RSA key A661100B3DAC1D4F2CAD8A54E603B2578FB8F0FB
gpg: Good signature from "Bryce Harrington <email address hidden>" [ultimate]
gpg: aka "Bryce Harrington <email address hidden>" [ultimate]
gpg: aka "Bryce Harrington <email address hidden>" [ultimate]

$ dput ubuntu ../logwatch_7.5.2-1ubuntu1.5_source.changes
D: Setting host argument.
Checking signature on .changes
gpg: ../logwatch_7.5.2-1ubuntu1.5_source.changes: Valid signature from E603B2578FB8F0FB
Checking signature on .dsc
gpg: ../logwatch_7.5.2-1ubuntu1.5.dsc: Valid signature from E603B2578FB8F0FB
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading logwatch_7.5.2-1ubuntu1.5.dsc: done.
  Uploading logwatch_7.5.2-1ubuntu1.5.debian.tar.xz: done.
  Uploading logwatch_7.5.2-1ubuntu1.5_source.buildinfo: done.
  Uploading logwatch_7.5.2-1ubuntu1.5_source.changes: done.
Successfully uploaded packages.

[Bryce Harrington (bryce)]

I see it listed in the unapproved queue currently:

    https://launchpad.net/ubuntu/focal/+queue?queue_state=1&queue_text=

Michal, please ping me if you don't see this in -proposed within a week or so, and we can investigate where it's stuck.

diff --git a/debian/changelog b/debian/changelog
index a861012..bd0073e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+logwatch (7.5.2-1ubuntu1.5) focal; urgency=medium
+
+  * d/p/focal-logwatch-ifconfig-fix.patch: Fix the if statement
+    in the code to let the code to select ip addr preferentially.
+    The SRU verification failed with previous version because the
+    if statement was inverted (LP: #1890748)
+
+ -- Michal Maloszewski <[e-mail]>  Thu, 17 Nov 2022 18:24:23 +0100
+
 logwatch (7.5.2-1ubuntu1.4) focal; urgency=medium
 
   * d/p/focal-logwatch-ifconfig-fix.patch: Fix the logwatch package
diff --git a/debian/patches/focal-logwatch-ifconfig-fix.patch b/debian/patches/focal-logwatch-ifconfig-fix.patch
index 45a5484..06cbf76 100644
--- a/debian/patches/focal-logwatch-ifconfig-fix.patch
+++ b/debian/patches/focal-logwatch-ifconfig-fix.patch
@@ -14,13 +14,13 @@ This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
  	my $cmd_to_show_int="";
  
 -        $cmd_to_show_int="$pathto_ifconfig -a";
-+        if ( -f $pathto_ip) {
-+         $cmd_to_show_int="$pathto_ifconfig -a";
-+	 }
-+	 else
-+	 {
-+	  $cmd_to_show_int=$pathto_ip." a";
-+	 }
++	if ( -f $pathto_ip) {
++	$cmd_to_show_int=$pathto_ip." a";
++	}
++	else
++	{
++	$cmd_to_show_int="$pathto_ifconfig -a";
++	}
  
          open(NET, "$cmd_to_show_int |")  || die "can't use $cmd_to_show_int: $!";
          while (<NET>)
diff --git a/debian/patches/series b/debian/patches/series
index 3c4d5b1..a37e37d 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,4 +1,3 @@
-focal-logwatch-ifconfig-fix.patch
 0001-00-debspecific-disable-su-reporting-in-secure.diff.patch
 0002-logfiles-vsftpd.conf-Use-custom-pattern-for-applystd.patch
 0003-Ignore-ecryptfs-automounting-messages-in-cron.patch
@@ -15,3 +14,4 @@ focal-logwatch-ifconfig-fix.patch
 0020-dhcpd-Ignore-lease-age-under-threshold-messages.patch
 0021-audit-use-the-term-ALLOWED-instead-of-Grants.patch
 allow-disabling-lookup.patch
+focal-logwatch-ifconfig-fix.patch