Re: [PATCH v2] maintenance tests: fix systemd v2.34.0-rc* test regression

Ævar Arnfjörð Bjarmason [off-list ref] writes:

On Wed, Nov 10 2021, Johannes Schindelin wrote:

quoted

Hi Ævar,

On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:

quoted

Fix tests added in b681b191f92 (maintenance: add support for systemd
timers on Linux, 2021-09-04) to run successfully on systems where
systemd-analyze is installed, but on which there's a discrepancy
between a FILE argument of "/lib/systemd/system/basic.target" and
"systemd/user/git-maintenance@.service" succeeding.

Could you try to rephrase `there's a discrepancy between a FILE argument
of "/lib/systemd/system/basic.target" and
"systemd/user/git-maintenance@.service" succeeding` more clearly?

Sure. Briefly to test if X works our prereq should test if X can work,
not if Y can work. Will try to update it.

Thanks.

quoted

quoted

 test_systemd_analyze_verify () {
-	if test_have_prereq SYSTEMD_ANALYZE
-	then
-		systemd-analyze verify "$@"
-	fi
+	# Ignoring any errors from systemd-analyze is intentional
+	systemd-analyze verify "$@" >systemd.out 2>systemd.err;

The semicolon is superfluous.

We use it in other places as shorthand for "I mean to not have && here".

That is news to me (not the presense of unnecessary semicolons; I do
not think we have "when you want to make it clear that you
deliberately did not write &&, write ;" in any of the guidelines,
and I am not sure if such a guideline is a good idea).

quoted

It is a bit sad that we're now doing unnecessary work when
`systemd-analyze` does not even exist.

It's a chicken & egg problem. How do you figure out if you're able to
run the command & get the output you expect without doing that?

I read "It is a bit sad" to imply "... but it probalby is the best
we can do, and more importantly, it is not worse than the problem it
is solving".

+		-e '/git-maintenance@i*\.service:/x' \

quoted

Lines containing `git-maintenance@.service:` (or the same pattern with an
arbitrary number of `i`s after the `@` character???) are exchanged with
hold space.

That does not look right.

It'll emit @.service or @i.service. I have no idea why, yeah the regex
is overly generous, but it doesn't hurt anything in this case (see
below)>

quoted

Since this is a `sed -n` call, we would need an explicit `p` command to
print anything. Therefore, the current code is a pretty expensive
equivalent to calling `true >&6`: it succeeds, and it does not print
anything.

Yes, like the buggy "if the prereq succeeds" code it replaced.

If both are buggy, then there is no point replacing old with new.
Let's have a non-buggy improved new and replace buggy old with it ;-)

Because I really don't know. Is it broken on literally one machine in
the world that I happened to have tested on, or more generally on the
sorts of OS version/whatever that has that systemd? No idea.

But the worst we'll do here is not run a test on some obscure setup.

Since the value of having the test is there if we just run it on some
setups that's fine. We should lean towards over-suppressing it to not
have "make test" in v2.34.0 fail on some platforms.

Hmph, if we _know_ (or _think_) the platforms that would fail a
particular test is rare enough *and* if we know we _don't_
understand why the test fails on them, wouldn't we want to know the
extent of the damage first by not suppressing the test and let it
break on these platforms that may help us after the release to
understand the breakage and deal with it?  The resolution may end up
to suppress the test after all, but with the approach we can do so
knowing why we are doing so, no?

This is different from OpenSSH 8.7 thing that we _know_ why the test
breaks, and we _know_ that there will be tons of instances with that
broken version.  Suppressing the test for them does make sense.

Thanks.