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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 10, 2021, 13:36 UTC
Message-ID: <nycvar.QRO.7.76.6.2111101422030.21127@tvgsbejvaqbjf.bet>
In-Reply-To: <patch-v2-1.1-44f0cafa16e-20211110T035103Z-avarab@gmail.com>

Hi Ævar,

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

Show 5 quoted lines

> 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?

Also, commit messages in git.git should not assume that every reader has a profound knowledge of `systemd`. The commit message needs to do a better job at explaining what is broken in the first place. The CI runs pass, after all, so it is unclear that there is anything broken that would need fixing to begin with.

Show 10 quoted lines

> There was an attempt to work around previous breakage in these tests
> in 670e5973992 (maintenance: fix test t7900-maintenance.sh,
> 2021-09-27), as noted in my [1] that commit is wrong about its
> assumption that we can use "/lib/systemd/system/basic.target" as a
> canary.argument.
>
> To fix this let's adjust this test to test what it really should be
> testing: If we've got systemd-analyze reporting anything useful, we
> should use it to check the syntax of our just-generated
> "systemd/user/git-maintenance@.service" file.

What is "anything useful" here? And how can we use the output of `systemd-analyze` to check the syntax of the generated `.service` file? This is all very unclear.

Show 10 quoted lines

> Even on systems where this previously succeeded we weren't effectively
> doing that, because "systemd-analyze" will pass various syntax errors
> by and exit with a status code of 0, e.g. if the "[Unit]" section is
> replaced with a nonsensical "[HlaghUnfUnf]" section.
>
> To do that ignore whatever exit code we get from "systemd-analyze
> verify", and filter its stderr output to extract the sorts of lines it
> emits on note syntax warnings and errors. We need to filter out
> "Failed to load", which would be emitted e.g. on the
> gcc135.fsffrance.org test box[1].

The domain name is not as interesting as the verbatim error message would be.

> We also need to pipe this output to FD's 5 & 6, to avoid mixing up the
> trace output with our own output under "-x".

We do not need to pipe to file descriptors 5 and 6. Other file descriptors would do, either. We need to redirect away from 1 and 2, is what this sentence should say.

And the hint about `-x` suggests that even that is not true that we need to redirect 1, either, just 2. And we would only need to redirect 2 with shells that do not understand `BASH_XTRACEFD`. It would be best not to mention `-x` at all.

Show 19 quoted lines

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 74aa6384755..5fe2ea03c1d 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -20,15 +20,16 @@ test_xmllint () {
>  	fi
>  }
>
> -test_lazy_prereq SYSTEMD_ANALYZE '
> -	systemd-analyze verify /lib/systemd/system/basic.target
> -'
> -
>  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.

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

> +
> +	cat systemd.out >&5 &&
> +	sed -n \
> +		-e '/^Failed to load/d' \

Lines starting with the prefix `Failed to load` are now deleted. Okay. Nothing in the commit explains why we can safely ignore those messages, though.

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

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.

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.

Show 15 quoted lines

> +		<systemd.err >&6 &&
> +	rm systemd.out systemd.err
>  }
>
>  test_expect_success 'help text' '
> @@ -697,7 +698,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
>  	# start registers the repo
>  	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
>
> -	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
> +	# If we have a systemd-analyze on the system we can verify the
> +	# generated file.
> +	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" 5>out 6>err &&
> +	test_must_be_empty out &&
> +	test_must_be_empty err &&

Since the function name has the suffix `_verify`, the verification part should be _inside_ that function, not outside.

This patch is not clear enough to tell whether it actually fixes a regression worth fast-tracking into v2.34.0 or not.

Ciao, Johannes

Show 7 quoted lines

>
>  	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>  	test_cmp expect args &&
> --
> 2.34.0.rc2.791.gdbfcf909579
>
>