Re: [PATCH 6/8] t: fix out-of-tree tests for some git-p4 tests

From: Toon Claes <toon@iotcl.com>
Date: Dec 13, 2024, 10:00 UTC
Message-ID: <87a5czj420.fsf@iotcl.com>
In-Reply-To: <20241211-pks-meson-ci-v1-6-28d18b494374@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

> Both t9835 and t9836 exercise git-p4, but one exercises Python 2 whereas
> the other one uses Python 3. These tests do not exercise "git p4", but
> instead they use "git p4.py" so that the unbuilt version of "git-p4.py"
> is used that has "#!/usr/bin/env python" as shebang instead of the
> replaced shebang.

It took me a while to figure out what you mean by "the replaced shebang"? I think you mean something like:

    These tests do not exercise "git p4", but instead they use "git
    p4.py". This calls the unbuilt version of "git-p4.py", which has the
    "#!/usr/bin/env python" shebang. This allows the test to modify
    which `python` comes first in $PATH, making it possible to force a
    Python version.

Show 5 quoted lines

> But "git-p4.py" is not in our PATH during out-of-tree builds, and thus
> we cannot locate "git-p4.py". The tests thus break with CMake and Meson.
>
> Fix this by instead manually setting up script wrappers that invoke the
> respective Python interpreter directly.

I like this approach, way more explicit now the Python version is in the command itself.

Show 13 quoted lines

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t9835-git-p4-metadata-encoding-python2.sh | 48 ++++++++++++++-------------
>  t/t9836-git-p4-metadata-encoding-python3.sh | 50 ++++++++++++++---------------
>  2 files changed, 50 insertions(+), 48 deletions(-)
>
> diff --git a/t/t9835-git-p4-metadata-encoding-python2.sh b/t/t9835-git-p4-metadata-encoding-python2.sh
> index 036bf79c6674f6f1f0d667c7270674168428ffee..02f9ec09053890a4d41b7dc95644066d6481bbb6 100755
> --- a/t/t9835-git-p4-metadata-encoding-python2.sh
> +++ b/t/t9835-git-p4-metadata-encoding-python2.sh
> @@ -14,23 +14,25 @@ python_target_version='2'
>  ## SECTION REPEATED IN t9836 ##

To be honest, I don't understand why this section wasn't put in a function in lib-git-p4.sh in the first place, instead of duplicating? Anyhow, I think for two test files it's fine to duplicate this code, and after all you're not changing that.

But I've noticed you are no longer using `python_target_version`. I would suggest to either remove the variable, or use it again so the code between the two test files is identical again. Doing the latter would probably mean you also need to create a variable like `p4_python=p4-python$python_target_version` and use `$p4_python` instead of `p4-python2` throughout the script, so I'm not sure that improves things.

Show 30 quoted lines

>  ###############################
>  
> -# Please note: this test calls "git-p4.py" rather than "git-p4", because the
> -# latter references a specific path so we can't easily force it to run under
> -# the python version we need to.
> -
> -python_major_version=$(python -V 2>&1 | cut -c  8)
> -python_target_binary=$(which python$python_target_version)
> -if ! test "$python_major_version" = "$python_target_version" && test "$python_target_binary"
> +# These tests are specific to Python 2. Write a custom script that executes
> +# git-p4 directly with the Python 2 interpreter to ensure that we use that
> +# version even if Git was compiled with Python 3.
> +python_target_binary=$(which python2)
> +if test -n "$python_target_binary"
>  then
>  	mkdir temp_python
> -	PATH="$(pwd)/temp_python:$PATH" && export PATH
> -	ln -s $python_target_binary temp_python/python
> +	PATH="$(pwd)/temp_python:$PATH"
> +	export PATH
> +
> +	write_script temp_python/git-p4-python2 <<-EOF
> +	exec "$python_target_binary" "$(git --exec-path)/git-p4" "\$@"
> +	EOF
>  fi
>  
> -python_major_version=$(python -V 2>&1 | cut -c  8)
> -if ! test "$python_major_version" = "$python_target_version"
> +git p4-python2 >err
> +if ! grep 'valid commands' err

I like this sanity check, this verifies if the command actually works:

Thus the output when the script is properly created:

    usage: /home/toon/devel/git/git-p4 <command> [options]

    valid commands: branches, clone, sync, submit, unshelve, commit, rebase

    Try /home/toon/devel/git/git-p4 <command> --help for command specific help.

And when the script was not written:

    git: 'p4-python2' is not a git command. See 'git --help'.

I noticed though, the stderr output isn's shallowed into /dev/null, resulting the output for the test to be the following if Python 2 is not found:

    make[2]: Entering directory '/home/toon/devel/git/t'
    *** t9835-git-p4-metadata-encoding-python2.sh ***
    which: no python2 in (/home/toon/devel/git/bin-wrappers:/home/toon/.local/bin:[snip])
    git: 'p4-python2' is not a git command. See 'git --help'.
    not ok 1 - start p4d

I think that's totally fine though, it's giving the user proper information about what is wrong.

-- Toon