Re: [PATCH v2 1/2] iotests: Require Python 3.6 or later

[Vladimir Sementsov-Ogievskiy]

20.09.2019 12:27, Kevin Wolf wrote:
> Am 20.09.2019 um 10:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 19.09.2019 19:29, Kevin Wolf wrote:
>>> Running iotests is not required to build QEMU, so we can have stricter
>>> version requirements for Python here and can make use of new features
>>> and drop compatibility code earlier.
>>>
>>> This makes qemu-iotests skip all Python tests if a Python version before
>>> 3.6 is used for the build.
>>>
>>> Suggested-by: Eduardo Habkost <address@hidden>
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>>    tests/qemu-iotests/check | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>> index 875399d79f..588c453a94 100755
>>> --- a/tests/qemu-iotests/check
>>> +++ b/tests/qemu-iotests/check
>>> @@ -633,6 +633,12 @@ then
>>>        export SOCKET_SCM_HELPER="$build_iotests/socket_scm_helper"
>>>    fi
>>>    
>>> +python_usable=false
>>> +if $PYTHON -c 'import sys; sys.exit(0 if sys.version_info >= (3,6) else 1)'
>>> +then
>>> +    python_usable=true
>>> +fi

don't we want
else
    PYTHON="false"
fi

to prevent some occasional unchecked call below?


>>> +
>>>    default_machine=$($QEMU_PROG -machine help | sed -n '/(default)/ s/ 
>>> .*//p')
>>>    default_alias_machine=$($QEMU_PROG -machine help | \
>>>       sed -n "/(alias of $default_machine)/ { s/ .*//p; q; }")
>>> @@ -809,7 +815,12 @@ do
>>>            start=$(_wallclock)
>>>    
>>>            if [ "$(head -n 1 "$source_iotests/$seq")" == "#!/usr/bin/env 
>>> python" ]; then
>>> -            run_command="$PYTHON $seq"
>>> +            if $python_usable; then
>>
>> hmm I wanted to say that this should not work, as python_usable is a
>> string. But I checked and see - it's work. Wow. Googled. And now I
>> understand that here "false" or "true" command is called, to obtain
>> it's return value.. I don't like bash and don't know its best
>> practice, but I'd prefer python_usable to be just return value of your
>> python command, like
>>
>> above:
>>
>>     $PYTHON -c 'import sys; sys.exit(0 if sys.version_info >= (3,6) else 1)'
>>     python_usable=$?
>>
>> and here:
>>
>>     if [ $python_usable -eq 0 ]; then
> 
> I'm just trying to be consistent with other variables used in the
> 'check' script. It has many boolean variables and they all end up
> calling the true/false commands.

Missed this, sorry. Then OK, of course..

still,
    echo "Unsupported Python version (must be >= 3.6)" > $seq.notrun
may be a bit more useful, if someone see it.


With or without any of suggestions:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>



-- 
Best regards,
Vladimir