Message ID | 20220614033223.377744-1-bauermann@kolabnow.com |
---|---|
State | Accepted |
Headers | show |
Series | [bug#55954] gnu: public-inbox: Fixes to allow the testsuite to run | expand |
Context | Check | Description |
---|---|---|
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
Hi, Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: > This patch makes the public-inbox testsuite pass. Some tests are skipped, > so the test coverage could likely be increased with more massaging. > > Perhaps the most significant change is using tini to run the testsuite so > that the testsuite's sub-processes are reaped. The ‘check’ phase is based on > the one from the mutter package. Thanks to Maxim Cournoyer for pointing out > this solution. > > * gnu/packages/patches/public-inbox-fix-spawn-test.patch: New file. > * gnu/local.mk (dist_patch_DATA): Add new patch. > * gnu/packages/mail.scm (public-inbox)[source]: Add new patch. > [arguments]<#:tests?>: Remove argument. > <#:imported-modules>: Add argument. > <#:modules>: Likewise. > <#:phases>{qualify-paths}: Substitute path for ‘/bin/cp’. > {pre-check}: Don't skip httpd-unix.t test. Remove unnecessary path > substitutions for “env” and “/bin/sh”. > {check}: Replace with custom version that launches the tests under tini. > [native-inputs]: Add tini. Applied, thanks! [...] > + (replace 'check > + (lambda* (#:key target > + (tests? (not target)) (test-flags '()) > + #:allow-other-keys) > + (if tests? > + (match (primitive-fork) > + (0 ;child process > + ;; lei tests build UNIX domain sockets in the temporary > + ;; directory, but the path of those sockets can be at most > + ;; 108 chars and Guix' default value for the variables > + ;; below already use 47 chars. Use the shortest temporary > + ;; path possible to avoid hitting the limit. > + (setenv "TEMP" "/tmp") > + (setenv "TEMPDIR" "/tmp") > + (setenv "TMP" "/tmp") > + (setenv "TMPDIR" "/tmp") > + > + ;; Use tini so that signals are properly handled and > + ;; doubly-forked processes get reaped; otherwise, > + ;; lei-daemon is kept as a zombie and the testsuite > + ;; fails thinking that it didn't quit as it should. > + (set-child-subreaper!) > + (apply execlp "tini" "--" > + "make" "check" test-flags)) > + (pid > + (match (waitpid pid) > + ((_ . status) > + (unless (zero? status) > + (error "`make check' exited with status" status)))))) It’s a bummer that we have to do all this. Would this work: (replace 'check (lambda _ (sigaction SIGINT SIG_DFL) (sigaction SIGTERM SIG_DFL) (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) ;; Ugly hack to make sure signal handler asyncs get a chance ;; to run while we’re in ‘waitpid’ during the “make check” ;; invocation. (sigaction SIGALRM (lambda _ (alarm 1))) (alarm 1))) ? If not, the solution you propose LGTM. Thanks, Ludo’.
Hello Ludo, Thank you for your review! Ludovic Courtès <ludo@gnu.org> writes: > Hi, > > Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: > >> This patch makes the public-inbox testsuite pass. Some tests are skipped, >> so the test coverage could likely be increased with more massaging. >> >> Perhaps the most significant change is using tini to run the testsuite so >> that the testsuite's sub-processes are reaped. The ‘check’ phase is based on >> the one from the mutter package. Thanks to Maxim Cournoyer for pointing out >> this solution. >> >> * gnu/packages/patches/public-inbox-fix-spawn-test.patch: New file. >> * gnu/local.mk (dist_patch_DATA): Add new patch. >> * gnu/packages/mail.scm (public-inbox)[source]: Add new patch. >> [arguments]<#:tests?>: Remove argument. >> <#:imported-modules>: Add argument. >> <#:modules>: Likewise. >> <#:phases>{qualify-paths}: Substitute path for ‘/bin/cp’. >> {pre-check}: Don't skip httpd-unix.t test. Remove unnecessary path >> substitutions for “env” and “/bin/sh”. >> {check}: Replace with custom version that launches the tests under tini. >> [native-inputs]: Add tini. > > Applied, thanks! I guess you had second thoughts. :-) >> + (replace 'check >> + (lambda* (#:key target >> + (tests? (not target)) (test-flags '()) >> + #:allow-other-keys) >> + (if tests? >> + (match (primitive-fork) >> + (0 ;child process >> + ;; lei tests build UNIX domain sockets in the temporary >> + ;; directory, but the path of those sockets can be at most >> + ;; 108 chars and Guix' default value for the variables >> + ;; below already use 47 chars. Use the shortest temporary >> + ;; path possible to avoid hitting the limit. >> + (setenv "TEMP" "/tmp") >> + (setenv "TEMPDIR" "/tmp") >> + (setenv "TMP" "/tmp") >> + (setenv "TMPDIR" "/tmp") >> + >> + ;; Use tini so that signals are properly handled and >> + ;; doubly-forked processes get reaped; otherwise, >> + ;; lei-daemon is kept as a zombie and the testsuite >> + ;; fails thinking that it didn't quit as it should. >> + (set-child-subreaper!) >> + (apply execlp "tini" "--" >> + "make" "check" test-flags)) >> + (pid >> + (match (waitpid pid) >> + ((_ . status) >> + (unless (zero? status) >> + (error "`make check' exited with status" status)))))) > > It’s a bummer that we have to do all this. Would this work: > > (replace 'check > (lambda _ > (sigaction SIGINT SIG_DFL) > (sigaction SIGTERM SIG_DFL) > (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) > > ;; Ugly hack to make sure signal handler asyncs get a chance > ;; to run while we’re in ‘waitpid’ during the “make check” > ;; invocation. > (sigaction SIGALRM (lambda _ (alarm 1))) > (alarm 1))) > > ? I assume there should be a "make check" in there somewhere. :-) Unfortunately, it didn't work. This is the check phase I tried: --8<---------------cut here---------------start------------->8--- (replace 'check (lambda _ (setenv "TEMP" "/tmp") (setenv "TEMPDIR" "/tmp") (setenv "TMP" "/tmp") (setenv "TMPDIR" "/tmp") (sigaction SIGINT SIG_DFL) (sigaction SIGTERM SIG_DFL) (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) ;; Ugly hack to make sure signal handler asyncs get a chance ;; to run while we’re in ‘waitpid’ during the “make check” ;; invocation. (sigaction SIGALRM (lambda _ (alarm 1))) (alarm 1) (unless (zero? (status:exit-val (system* "make" "check"))) (error "`make check' exited with error")))) --8<---------------cut here---------------end--------------->8--- And this was the end of the output of the phase: --8<---------------cut here---------------start------------->8--- ⋮ Bailout called. Further testing stopped: daemon still running (PID:5282) FAILED--Further testing stopped: daemon still running (PID:5282) make: *** [Makefile:4141: check-each] Error 255 error: in phase 'check': uncaught exception: misc-error #f "~A" ("`make check' exited with error") #f phase `check' failed after 27.7 seconds --8<---------------cut here---------------end--------------->8--- “daemon still running” means that lei's daemon process wasn't reaped. > If not, the solution you propose LGTM. Thanks. Hopefully this can be fixed in Guix's build machinery itself in the core-updates branch and all the packages that are using this hack can drop it then.
Hello, Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: > Ludovic Courtès <ludo@gnu.org> writes: [...] >> Applied, thanks! > > I guess you had second thoughts. :-) Yes, sorry; I wasn’t sure whether bothering you was a good idea, but I was also dissatisfied with that whole situation (which is not public-inbox’s fault!). >> It’s a bummer that we have to do all this. Would this work: >> >> (replace 'check >> (lambda _ >> (sigaction SIGINT SIG_DFL) >> (sigaction SIGTERM SIG_DFL) >> (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) >> >> ;; Ugly hack to make sure signal handler asyncs get a chance >> ;; to run while we’re in ‘waitpid’ during the “make check” >> ;; invocation. >> (sigaction SIGALRM (lambda _ (alarm 1))) >> (alarm 1))) >> >> ? > > I assume there should be a "make check" in there somewhere. :-) > > Unfortunately, it didn't work. This is the check phase I tried: > > (replace 'check > (lambda _ > (setenv "TEMP" "/tmp") > (setenv "TEMPDIR" "/tmp") > (setenv "TMP" "/tmp") > (setenv "TMPDIR" "/tmp") > > (sigaction SIGINT SIG_DFL) > (sigaction SIGTERM SIG_DFL) > (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) > > ;; Ugly hack to make sure signal handler asyncs get a chance > ;; to run while we’re in ‘waitpid’ during the “make check” > ;; invocation. > (sigaction SIGALRM (lambda _ (alarm 1))) > (alarm 1) > (unless (zero? (status:exit-val (system* "make" "check"))) > (error "`make check' exited with error")))) > > > And this was the end of the output of the phase: > > ⋮ > Bailout called. Further testing stopped: daemon still running (PID:5282) > FAILED--Further testing stopped: daemon still running (PID:5282) > make: *** [Makefile:4141: check-each] Error 255 > error: in phase 'check': uncaught exception: > misc-error #f "~A" ("`make check' exited with error") #f > phase `check' failed after 27.7 seconds > > “daemon still running” means that lei's daemon process wasn't reaped. Damnit, I wonder how that can happen. >> If not, the solution you propose LGTM. > > Thanks. Hopefully this can be fixed in Guix's build machinery itself in > the core-updates branch and all the packages that are using this hack > can drop it then. Yeah. Anyway, applied for good this time, thanks for taking extra time on this! Ludo’.
diff --git a/gnu/local.mk b/gnu/local.mk index 03e180cc8508..0653cbb3bc16 100644 --- a/gnu/local.mk +++ b/gnu/local.mk @@ -1650,6 +1650,7 @@ dist_patch_DATA = \ %D%/packages/patches/psm-disable-memory-stats.patch \ %D%/packages/patches/psm-ldflags.patch \ %D%/packages/patches/psm-repro.patch \ + %D%/packages/patches/public-inbox-fix-spawn-test.patch \ %D%/packages/patches/pulseaudio-fix-mult-test.patch \ %D%/packages/patches/pulseaudio-longer-test-timeout.patch \ %D%/packages/patches/pulseview-qt515-compat.patch \ diff --git a/gnu/packages/mail.scm b/gnu/packages/mail.scm index 8596d0808beb..6f44c6c0f56a 100644 --- a/gnu/packages/mail.scm +++ b/gnu/packages/mail.scm @@ -87,6 +87,7 @@ (define-module (gnu packages mail) #:use-module (gnu packages django) #:use-module (gnu packages dns) #:use-module (gnu packages docbook) + #:use-module (gnu packages docker) #:use-module (gnu packages documentation) #:use-module (gnu packages emacs) #:use-module (gnu packages enchant) @@ -4084,10 +4085,16 @@ (define-public public-inbox (sha256 (base32 "0xni1l54v1z3p0zb52807maay0yqabp8jgf5iras5zmhgjyk3swz")) - (file-name (git-file-name name version)))) + (file-name (git-file-name name version)) + (patches (search-patches "public-inbox-fix-spawn-test.patch")))) (build-system perl-build-system) (arguments - '(#:tests? #f + `(#:imported-modules (,@%perl-build-system-modules + (guix build syscalls)) + #:modules ((guix build perl-build-system) + (guix build syscalls) + (guix build utils) + (ice-9 match)) #:phases (modify-phases %standard-phases (add-before 'configure 'qualify-paths @@ -4096,18 +4103,45 @@ (define-public public-inbox (substitute* "lib/PublicInbox/Xapcmd.pm" (("'xapian-compact'") (format #f "'~a'" (search-input-file inputs - "/bin/xapian-compact")))))) + "/bin/xapian-compact")))) + (substitute* "lib/PublicInbox/TestCommon.pm" + ;; This is only used for tests, but get it from ‘inputs’ so + ;; that cross builds won't hold a reference to a package built + ;; for another architecture. + (("/bin/cp") (search-input-file inputs "/bin/cp"))))) (add-before 'check 'pre-check (lambda _ - (substitute* "t/spawn.t" - (("\\['env'\\]") (string-append "['" (which "env") "']"))) - (substitute* "t/ds-leak.t" - (("/bin/sh") (which "sh"))) - (invoke "./certs/create-certs.perl") - ;; XXX: This test fails due to zombie process is not reaped by - ;; the builder. - (substitute* "t/httpd-unix.t" - (("^SKIP: \\{") "SKIP: { skip('Guix');")))) + (invoke "./certs/create-certs.perl"))) + (replace 'check + (lambda* (#:key target + (tests? (not target)) (test-flags '()) + #:allow-other-keys) + (if tests? + (match (primitive-fork) + (0 ;child process + ;; lei tests build UNIX domain sockets in the temporary + ;; directory, but the path of those sockets can be at most + ;; 108 chars and Guix' default value for the variables + ;; below already use 47 chars. Use the shortest temporary + ;; path possible to avoid hitting the limit. + (setenv "TEMP" "/tmp") + (setenv "TEMPDIR" "/tmp") + (setenv "TMP" "/tmp") + (setenv "TMPDIR" "/tmp") + + ;; Use tini so that signals are properly handled and + ;; doubly-forked processes get reaped; otherwise, + ;; lei-daemon is kept as a zombie and the testsuite + ;; fails thinking that it didn't quit as it should. + (set-child-subreaper!) + (apply execlp "tini" "--" + "make" "check" test-flags)) + (pid + (match (waitpid pid) + ((_ . status) + (unless (zero? status) + (error "`make check' exited with status" status)))))) + (format #t "test suite not run~%")))) (add-after 'install 'wrap-programs (lambda* (#:key inputs outputs #:allow-other-keys) (let ((out (assoc-ref outputs "out"))) @@ -4126,7 +4160,7 @@ (define-public public-inbox (find-files (string-append out "/bin"))))))))) (native-inputs (list ;; For testing. - lsof openssl)) + lsof openssl tini)) (inputs (list bash-minimal curl diff --git a/gnu/packages/patches/public-inbox-fix-spawn-test.patch b/gnu/packages/patches/public-inbox-fix-spawn-test.patch new file mode 100644 index 000000000000..2739b1974de8 --- /dev/null +++ b/gnu/packages/patches/public-inbox-fix-spawn-test.patch @@ -0,0 +1,43 @@ +From 5593489d9c3ce22b1942f35c7ebb0e06fcf2bfa8 Mon Sep 17 00:00:00 2001 +From: Thiago Jung Bauermann <bauermann@kolabnow.com> +Date: Fri, 10 Jun 2022 12:39:18 -0300 +Subject: [PATCH] t/spawn: Find invalid PID to try to join its process group + +In the container used to build packages of the GNU Guix distribution, PID 1 +runs as the same user as the test so this spawn that should fail actually +succeeds. + +Fix the problem by going through different PIDs and picking one that +either doesn't exist or we aren't allowed to signal. +--- + +This patch is taken from the public-inbox repository and will appear in the +release after v1.8. + + t/spawn.t | 13 ++++++++++++- + 1 file changed, 12 insertions(+), 1 deletion(-) + +diff --git a/t/spawn.t b/t/spawn.t +index 6168c1f6171c..5fc99a2a101c 100644 +--- a/t/spawn.t ++++ b/t/spawn.t +@@ -24,7 +24,18 @@ SKIP: { + is(waitpid($pid, 0), $pid, 'waitpid succeeds on spawned process'); + is($?, 0, 'true exited successfully'); + pipe(my ($r, $w)) or BAIL_OUT; +- $pid = eval { spawn(['true'], undef, { pgid => 1, 2 => $w }) }; ++ ++ # Find invalid PID to try to join its process group. ++ my $wrong_pgid = 1; ++ for (my $i=0x7fffffff; $i >= 2; $i--) { ++ if (kill(0, $i) == 0) { ++ $wrong_pgid = $i; ++ last; ++ } ++ } ++ ++ # Test spawn behavior when it can't join the requested process group. ++ $pid = eval { spawn(['true'], undef, { pgid => $wrong_pgid, 2 => $w }) }; + close $w; + my $err = do { local $/; <$r> }; + # diag "$err ($@)";