Code review comment for ~liushuyu-011/ubuntu/+source/spamassassin:ubuntu/devel

Hi Zixing,

Thanks for fixing this problem, but in the future please can you also
ensure there is a bug report associated with the change? That is
important not just for tracking, but also for documenting what the
original problem was that necessitated the change (I am guessing a dep8
test failure?) and to explain the approach used for fixing it.

In particular, this is important for fixes originating from the Ubuntu
side, in order to facilitate forwarding them upstream at some point. In
particular, this patch in particular looks like it should go upstream,
so I'd like to know if you didn't forward it upstream yourself, what the
reasons were?

Thanks again for this contribution, it looks like a solid fix and I
appreciate that you caught it during -devel.

Bryce

----- Forwarded message from Zixing Liu <email address hidden> -----

Date: Thu, 21 Mar 2024 04:22:20 -0000
From: Zixing Liu <email address hidden>
To: <email address hidden>
Subject: [Merge] ~liushuyu-011/::/spamassassin:ubuntu/devel into ::/spamassassin:ubuntu/devel
Reply-To: <email address hidden>

Zixing Liu has proposed merging ~liushuyu-011/ubuntu/+source/spamassassin:ubuntu/devel into ubuntu/+source/spamassassin:ubuntu/devel.

Requested reviews:
  Ubuntu Sponsors (ubuntu-sponsors)

For more details, see:
https://code.launchpad.net/~liushuyu-011/ubuntu/+source/spamassassin/+git/spamassassin/+merge/462834

This MP fixes the DNS unit tests in Spamassassin, where the built-in DNS server failed to start due to an API change in the Net::DNS::Nameserver module.
--
Your team git-ubuntu developers is subscribed to branch ubuntu/+source/spamassassin:ubuntu/devel.

diff --git a/debian/changelog b/debian/changelog
index 406f4ee..1430ee5 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+spamassassin (4.0.0-8ubuntu3~ppa1) noble; urgency=medium
+
+ * debian/patches/t-adapt-to-new-dns-module-behavior.patch: Adapt to
+ new Net::DNS::Nameserver behavior.
+
+ -- Zixing Liu <email address hidden> Wed, 20 Mar 2024 22:19:39 -0600
+
 spamassassin (4.0.0-8ubuntu2) noble; urgency=medium

   * No-change rebuild against libssl3t64
diff --git a/debian/patches/series b/debian/patches/series
index 991acfd..994a39f 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -12,3 +12,4 @@ spamd_disable_ai_addrconfig_for_ipv4_literals.patch
 bug1041759-deprecated-method.patch
 bug1039960-deprecated-method-rdstring.patch
 bug_1056001-reread-prefs.patch
+t-adapt-to-new-dns-module-behavior.patch
diff --git a/debian/patches/t-adapt-to-new-dns-module-behavior.patch b/debian/patches/t-adapt-to-new-dns-module-behavior.patch
new file mode 100644
index 0000000..dd2d435
--- /dev/null
+++ b/debian/patches/t-adapt-to-new-dns-module-behavior.patch
@@ -0,0 +1,69 @@
+Description: Adapt to new Net::DNS::Nameserver behavior
+ The newer version will do fork() itself and will break
+ if doing a double-fork from the tests.
+Author: Zixing Liu <email address hidden>
+Forwarded: no
+Last-Update: 2024-03-21
+
+--- spamassassin-4.0.0.orig/t/dnsbl_subtests.t
++++ spamassassin-4.0.0/t/dnsbl_subtests.t
+@@ -221,7 +221,8 @@ sub dns_server($$) {
+ LocalAddr => $local_addr, LocalPort => $local_port,
+ ReplyHandler => \&reply_handler, Verbose => 0);
+ $ns or die "Cannot create a nameserver object";
+- $ns->main_loop;
++ $ns->start_server(10);
++ return $ns;
+ }
+
+ sub find_free_port($) {
+@@ -308,16 +309,7 @@ if ($sock_tcp) {
+ $sock_tcp->close() or die "Error closing TCP socket: $!";
+ }
+
+-# detach a DNS server process
+-my $pid = fork();
+-defined $pid or die "Cannot fork: $!";
+-if (!$pid) { # child
+- dns_server($dns_server_localaddr, $dns_server_localport);
+- exit;
+-}
+-
+-# parent
+-# print STDERR "Forked a DNS server process [$pid]\n";
++my $ns = dns_server($dns_server_localaddr, $dns_server_localport);
+ sleep 1;
+
+ $spamassassin_obj = Mail::SpamAssassin->new({
+@@ -353,28 +345,9 @@ test_samples(
+ # X_URIBL_Y_FFD no longer hits intentionally (not in the 127.0.0.0/8 range),
+ # see Bug 6803
+
+-if ($pid) {
+- kill('TERM',$pid) or die "Cannot stop a DNS server [$pid]: $!";
+-
+-# Bug 7000: Seems like a DNS server process can't be terminated. [...]
+-# Reason is "waitpid($pid,0)". If commented out, it does not hang.
+-# There are no extra processes after end of this test.
+-#
+-# perlfunc: waitpid - waiting for a particular pid with FLAGS of 0 is
+-# implemented everywhere
+-#
+-# perlport: (Win32) waitpid Can only be applied to process handles returned
+-# for processes spawned using "system(1, ...)" or pseudo processes created
+-# with "fork()".
+-#
+-# so ... waitpid($pid,0) should work on Windows, but it doesn't - nevermind:
+-
+- waitpid($pid,0) unless $RUNNING_ON_WINDOWS;
+-
+- undef $pid;
+-}
+-
+ END {
+ $spamassassin_obj->finish if $spamassassin_obj;
+- kill('KILL',$pid) if $pid; # ignoring status
++ if ($ns) {
++ $ns->stop_server();
++ }
+ }

----- End forwarded message -----