Merge ~mirespace/ubuntu/+source/squid-deb-proxy:sru-focal-squid-deb-proxy-lp1505670-apt-avahi-discover into ubuntu/+source/squid-deb-proxy:ubuntu/focal-devel

[Miriam España Acebal]

Hello team,

PPA with the package is here: https://launchpad.net/~mirespace/+archive/ubuntu/sru-focal-squid-deb-proxy-lp1505670-apt-avahi-discover.

This fix for LP#1505670 is cherry-picked from upstream (https://github.com/mvo5/squid-deb-proxy/commit/604ba3f98beff25a8fd51783d3ffc4db5e987dab by @mvo).

Steps for reproducing were set by Christian (paelzer) in the Bug's description and you can find them as well in the SRU template (with the bad and the good case -the good case without extra ifaces is easy to test-).

The build was ok, only lintian warnings:

W: squid-deb-proxy: missing-systemd-service-for-init.d-script squid-deb-proxy
W: squid-deb-proxy changes: unknown-architecture amd64_translations

I: Lintian run was successful.

No tests in the package are included.

Thanks a lot for your time here... ready to review.

[Sergio Durigan Junior (sergiodj)]

Thank you for the MP, Miriam.

Marking it as Needs Information as per https://code.launchpad.net/~mirespace/ubuntu/+source/squid-deb-proxy/+git/squid-deb-proxy/+merge/410288/comments/1083557. Those comments also apply to this MP.

review: Needs Information

[Miriam España Acebal (mirespace)]

Hi Sergio!

Thank you for your review :).

In fact, I was thinking that those lintian errors were there because it's a sync for now, but I thought about the "boy scout rule" (Always leave the code better than you found it) and more in this case that it's a SRU (and for the older one, Bionic). But better know for sure with your comment that is not strictly necessary.

Definitely yes to the PPA thing. Indeed, I see it when testing an open-vm-tools for Christian and I realized that "it was the way" (great point for the versions conflict hint), but I had the impish ppa for this already and the one for hirsute, so I continued for these bugs with the separates ppa.

After the talk on MM and your steps to reproduce (thanks!):
 - I mixed them with the ones for Christian and I think is now better and clearer I think (I'm afraid, not shorter).
 - On how far to go with the SRUs: I think is related to (or derived better said) the nuance that the conversation took on. Your point looks like very sensate, but I don't see myself in the position to make that decision.

Thanks a lot Sergio!

[Miriam España Acebal (mirespace)]

Hi again!

I changed the error's output as per Robie's suggestion (using now stderr). Updated changelog as well to reflect that the solution it's different from the one in the stable release.

I checked that the package's behaviour (fix) is the same with this new change but, please, take a look at it just in case.

New package uploaded to the ppa with -focalppa1 suffix.

Thanks in advance!

[Sergio Durigan Junior (sergiodj)]

Hi Miriam,

Thanks for following up on this :-). I've tested the new package/approach and it works as expected indeed. Upon reading more of the discussion on the bug and thinking about our conversation earlier, I came to the conclusion that I personally wouldn't mind to backport the original patch which sends the message to syslog, but oh well, now you've already done all the work so that's totally fine :-).

I only found a very small nit in the d/changelog entry that I think should be fixed (I'm leaving an inline comment), but otherwise the MP LGTM and is ready to be sponsored. Please, let me know when you address the nit below (which applies to the other MPs as well, by the way) and I will be happy to upload the package for you.

Thanks again!

[Sergio Durigan Junior (sergiodj)]

Ah, sorry, I forgot to mention: I have a few comments about the SRU template, specifically the Test Plan section.

I was following it in order to set everything up and try to reproduce the bug & verify the fix, and I noticed that there are a few things missing:

1) After setting up the test-bug1505670-focal VM, it's not clear which packages the user needs to install in order to fully reproduce the issue. ISTR that this information was available before. Would you mind adding it back, please? You need to put it after the "sudo apt update" command.

2) I think the text could also be clearer regarding which commands need to be executed on the host vs. inside the VM. For example, the "apt install apt-cacher-ng" needs to be done on the host.

I think with these improvements the text will look better. Thanks.

[Miriam España Acebal (mirespace)]

Hi Sergio!

Changes are done... all good proposals from you :). Crossing fingers I don't miss anything.

[Sergio Durigan Junior (sergiodj)]

Thanks, Miriam :-). It's looking really great now, so I will sponsor the upload.

Just a very minor detail: we almost always refer to bugs between parentheses in the changelog entry. So in this case, it would be:

  * apt-avahi-discover: write errors to stderr instead
    of stdout so that stdout can be captured correctly.
    Thanks to <email address hidden>. Note that this is
    being fixed differently (stderr and not syslog)
    in the stable releases. (LP: #1505670)

But that's OK and you don't need to worry about it this time :-).

Thanks again.

Uploaded:

$ dput squid-deb-proxy_0.8.15ubuntu0.20.04.1_source.changes
Trying to upload package to ubuntu
Checking signature on .changes
gpg: /home/sergio/work/squid-deb-proxy/squid-deb-proxy_0.8.15ubuntu0.20.04.1_source.changes: Valid signature from 106DA1C8C3CBBF14
Checking signature on .dsc
gpg: /home/sergio/work/squid-deb-proxy/squid-deb-proxy_0.8.15ubuntu0.20.04.1.dsc: Valid signature from 106DA1C8C3CBBF14
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading squid-deb-proxy_0.8.15ubuntu0.20.04.1.dsc: done.
  Uploading squid-deb-proxy_0.8.15ubuntu0.20.04.1.tar.xz: done.
  Uploading squid-deb-proxy_0.8.15ubuntu0.20.04.1_source.buildinfo: done.
  Uploading squid-deb-proxy_0.8.15ubuntu0.20.04.1_source.changes: done.
Successfully uploaded packages.

review: Approve

diff --git a/apt-avahi-discover b/apt-avahi-discover
index dfd579b..65388d6 100755
--- a/apt-avahi-discover
+++ b/apt-avahi-discover
@@ -58,6 +58,11 @@ class AptAvahiClient(asyncore.dispatcher):
     def __repr__(self):
         return "<{}> {}: {}".format(
             self.__class__.__name__, self.addr, self.time_to_connect)
+    def log(self, message):
+        sys.stderr.write('log: %s\n' % str(message))
+    def log_info(self, message, type='info'):
+        if type not in self.ignore_log_types:
+            self.log('%s: %s' % (type, message))
 
 
 def is_ipv6(a):
diff --git a/debian/changelog b/debian/changelog
index 6072002..3656008 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+squid-deb-proxy (0.8.15ubuntu0.20.04.1) focal; urgency=medium
+
+  * apt-avahi-discover: write errors to stderr instead
+    of stdout so that stdout can be captured correctly.
+    Thanks to <[e-mail]>. Note that this is
+    being fixed differently (stderr and not syslog)
+    in the stable releases. LP: #1505670.
+
+ -- Miriam España Acebal <[e-mail]>  Thu, 14 Oct 2021 13:41:22 +0200
+
 squid-deb-proxy (0.8.15) unstable; urgency=medium
 
   [ Graham Cantin ]
diff --git a/debian/control b/debian/control
index d7c877a..abdd386 100644
--- a/debian/control
+++ b/debian/control
@@ -1,7 +1,8 @@
 Source: squid-deb-proxy
 Section: net
 Priority: optional
-Maintainer: Michael Vogt <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: Michael Vogt <[e-mail]>
 Build-Depends: debhelper (>= 12),
                gettext,
                intltool,