Re: [Fuego] [PATCH] python3: Use python3 in the scripts to fully migrate

Hi Tim,

Sorry for the late, I am on vacation last week.

Please find my answers below.

>-----Original Message-----
>From: Bird, Tim <[email protected]>
>Sent: 05 October 2022 05:52
>To: pyla venkata(=1B$B#T#S#I#P=1B(B TMIEC ODG Porting) <Venkata.Pyla@toshi=
ba-
>tsip.com>; [email protected]
>Cc: nakkala sireesha(=1B$B#T#S#I#P=1B(B TMIEC ODG Porting) <sireesha.nakka=
la@toshiba-
>tsip.com>; dinesh kumar(=1B$B#T#S#I#P=1B(B TMIEC ODG Porting) <dinesh.kuma=
r@toshiba-
>tsip.com>; hayashi kazuhiro(=1B$BNS=1B(B =1B$BOB9(=1B(B =1B$B""#S#W#C"~#A#=
C#T=1B(B)
><[email protected]>
>Subject: RE: [PATCH] python3: Use python3 in the scripts to fully migrate
>
>OK -  I have some more comments on this patch.
>
>See inline below.
>
>> -----Original Message-----
>> From: [email protected] <[email protected]>
>>
>> The python scripts are failing to run in an environment where `python`
>> is not defined, though the scripts are migrated to work on python3 the
>> shebang is still pointing to python which may be undefined
>>
>> Signed-off-by: venkata pyla <[email protected]>
>> ---
>>  scripts/check-dependencies              | 2 +-
>
>My first comment is that invocation using just the interpreter name of 'py=
thon' is
>in a lot more places than just these 4 files, and the parser.py scripts.
>
>I see it also in deorphan-runs.py, gen-page.py, jdiff, test_parser.sh ,
>generic_parser.py, test_filelock.py (in the scripts directory).  Many of t=
hese you
>are likely not using.
Yes, many of the files were not used by Toshiba, so we fixed only in the te=
sts where it is breaking.

>
>However, some of these are intended for use by Fuego users, for special
>circumstances (like deorphan-runs.py and test_parser.sh).


>
>>  scripts/common.sh                       | 8 ++++----
>>  scripts/ftc                             | 2 +-
>>  scripts/functions.sh                    | 2 +-
>>  tests/Benchmark.Dhrystone/parser.py     | 2 +-
>>  tests/Benchmark.IOzone/parser.py        | 2 +-
>>  tests/Benchmark.Stream/parser.py        | 2 +-
>>  tests/Benchmark.bonnie/parser.py        | 2 +-
>>  tests/Benchmark.cyclictest/parser.py    | 2 +-
>>  tests/Benchmark.fio/parser.py           | 2 +-
>>  tests/Benchmark.hackbench/parser.py     | 2 +-
>>  tests/Benchmark.lmbench2/parser.py      | 2 +-
>>  tests/Benchmark.migratetest/parser.py   | 2 +-
>>  tests/Benchmark.pmqtest/parser.py       | 2 +-
>>  tests/Benchmark.ptsematest/parser.py    | 2 +-
>>  tests/Benchmark.signaltest/parser.py    | 2 +-
>>  tests/Benchmark.sigwaittest/parser.py   | 2 +-
>>  tests/Benchmark.svsematest/parser.py    | 2 +-
>>  tests/Functional.LTP/fuego_test.sh      | 4 ++--
>>  tests/Functional.LTP/ltp_process.py     | 2 +-
>>  tests/Functional.LTP/parser.py          | 2 +-
>>  tests/Functional.LTP_one_test/parser.py | 2 +-
>> tests/Functional.autopkgtest/parser.py  | 2 +-
>>  tests/Functional.linaro/parser.py       | 2 +-
>>  24 files changed, 28 insertions(+), 28 deletions(-)
>>
>> diff --git a/scripts/check-dependencies b/scripts/check-dependencies
>> index 583012e..7369bd4 100755
>> --- a/scripts/check-dependencies
>> +++ b/scripts/check-dependencies
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>
>check-dependencies is used by Functional.LTP, so I'm surprised you haven't=
 run
>into this one.  Maybe you have and this program works fine with python3.
We use LTP test and so we changed the shebang here, and as you suggested be=
low it is good to use `run_python` which will internally find the right int=
erpreter as you wrote the code, then this shebang is not required or we can=
 remove.

>
>check-dependencies is invoked directly (not using run_python), or by calli=
ng the
>interpreter, but it is only called in one place (Functional.LTP/fuego_test=
.sh), so
>maybe that one place could call run_python, to make it so that this would =
work
>with a conditional interpreter.
>(see below for how I might make common.sh auto-detect the python interpret=
er)
This will be a good solution for the backward compatibility.

>
>>  # vim: set ts=3D4 sw=3D4 et :
>>  #
>>  # check_dependencies - check config dependencies listed in a file
>> diff --git a/scripts/common.sh b/scripts/common.sh index
>> f916d84..cf93ddb 100644
>> --- a/scripts/common.sh
>> +++ b/scripts/common.sh
>> @@ -108,20 +108,20 @@ function run_python() {
>>      if [ ! -z $ORIG_PATH ] ; then
>>          dprint "run_python with PATH=3D$ORIG_PATH, TOOLCHAIN=3D$TOOLCHA=
IN"
>>          export TOOLCHAIN
>> -        PATH=3D$ORIG_PATH TOOLCHAIN=3D$TOOLCHAIN python "$@"
>> +        PATH=3D$ORIG_PATH TOOLCHAIN=3D$TOOLCHAIN python3 "$@"
>For this one and the next, I'm thinking of using a variable to indicate th=
e
>interpreter:
>PYTHON_EXE, that I would detect outside of the run_python function, with
>something like this:
>
>INTERPRETER_LIST=3D"python3 python python2"
>for interpreter in $INTERPRETER_LIST ; do
>    if [ -x /usr/bin/${interpreter} ] ; then
>        PYTHON_EXE=3D"/usr/bin/${interpreter}"
>        break
>    fi
>done
>if [ -z "$PYTHON_EXE" ] ; then
>    abort_job "No python interpreter found!"
>fi
>
>this line would then become:
>PATH=3D$ORIG_PATH TOOLCHAIN=3D$TOOLCHAIN $PYTHON_EXE "$@"
>
>Note the use of $ORIG_PATH.  Some toolchain setup scrips modify the PATH s=
o
>that the 'python' that is executed is one that the SDK for the toolchain s=
et up, with
>libs from the target sysroot.  This is not desired for Fuego's use of pyth=
on, which
>should always use the host's python interpreter.  I'll have to see if this=
 proposed
>change (which would use the fullpath to the interpreter, instead of just t=
he
>interpreter found in the PATH), would affect this use of ORIG_PATH with so=
me
>toolchains.
>
>Let me know if you have any thoughts about this.
>Specifically, does your toolchain setup script set ORIG_PATH?
We are not using the ORIG_PATH variable, in our case the host environment i=
nterpreter is sufficient to run the scripts.

But in our patch we have modified in both the places when ORIG_PATH is defi=
ned and not defined because we wanted at least change python version proble=
m in the same file.

>
>>      else
>>          dprint "run_python with TOOLCHAIN=3D$TOOLCHAIN"
>>          export TOOLCHAIN
>> -        TOOLCHAIN=3D$TOOLCHAIN python "$@"
>> +        TOOLCHAIN=3D$TOOLCHAIN python3 "$@"
>and this would become:
>TOOLCHAIN=3D$TOOLCHAIN $PYTHON_EXE "$@"
>
>>      fi
>>  }
>>
>>  function run_python_quiet() {
>>      if [ ! -z $ORIG_PATH ]
>>      then
>> -        PATH=3D$ORIG_PATH python "$@"
>> +        PATH=3D$ORIG_PATH python3 "$@"
>As near as I can tell, run_python_quiet is only ever used by overlays/base=
/base-
>params.fuegoclass
>(and it shows up in expected values in Functional.fuego_ftc_test).
>This is used to invoke sercp, serlogin, and sersh for serial port connecti=
ons to the
>target.
>(that is, when the TRANSPORT=3Dserial)
>
>I have NOT tested sercp, serlogin and sersh for python3 compatibility (but=
 I
>already know they are NOT python3 compatible right now).  Given that they =
are
>managing serial port data flow on the  serial port at a byte-at-a-time lev=
el, they
>will need extensive analysis to make sure that the string handling does no=
t break
>when they are run with python3 (where strings default to Unicode instead o=
f byte
>strings).
>I have another project on github that also handles data flow on a serial p=
ort, and
>it was very difficult to make it work correctly on both python2 and python=
3.
>This one I'll have to defer.
>
>If I understand correctly, the way that you use Fuego is by installing it =
natively
>onto the device under test, and using TRANSPORT=3Dlocal.  If you don't use=
 the
>serial TRANSPORT or the serial port tools (serio suite of tools), then thi=
s shouldn't
>affect you.
Yes, this change is not required for us, as mentioned above we modified all=
 python versions in the same file.
You can ignore this change and thanks for letting me know about the functio=
n 'run_python_quiet'

>
>In any event, this can not be changed to python3 yet.
>>      else
>> -        python "$@"
>> +        python3 "$@"
>Nor this one. (see above)
>
>>      fi
>>  }
>>
>> diff --git a/scripts/ftc b/scripts/ftc index adce17b..4c5ff7e 100755
>> --- a/scripts/ftc
>> +++ b/scripts/ftc
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>
>Have you been running ftc with this change already?
>
>ftc has a large number of dependencies on python modules, especially in th=
e area
>of report generation.  The docker container never installs python3 version=
s of
>some of these esoteric modules, such as reportlab and openpyxl.
>
>Just out of curiosity, do you import these modules in your environment, or=
 just not
>use the ftc functionality that depends on them?
We installed python3 dependencies in our target machine.
as you may know we are not using the docker-container and installing the fu=
ego-core directly on the target machine, and the python3 dependencies are a=
lready installed in the target machine.

>
>Also, ftc has a few remaining python3 incompatibilities:
>  -  raw_input vs input
>  - some urllib-related fixes, for the python3 refactoring of ulrparse and=
 urllib
>
>Is ftc functioning correctly with python3 now, for all your usage scenario=
s?
It is working fine with our current usage scenarios, because in our environ=
ment we were not using the Jenkins or the features related to those functio=
ns
I think we should fix them because they are obviously required when complet=
ely migrate to pyhon3.

>
>>  #
>>  # vim: set ts=3D4 sw=3D4 et :
>>  #
>> diff --git a/scripts/functions.sh b/scripts/functions.sh index
>> 7afe423..5a6e0e5 100755
>> --- a/scripts/functions.sh
>> +++ b/scripts/functions.sh
>> @@ -506,7 +506,7 @@ function build {
>>          call_if_present test_build
>>          ret=3D$?
>>          build_end_time=3D$(date +"%s.%N")
>> -        build_duration=3D$(python -c "print($build_end_time - $build_st=
art_time)")
>> +        build_duration=3D$(python3 -c "print($build_end_time -
>> + $build_start_time)")
>If I do the PYTHON_EXE thing in common.sh, I should be able to use that he=
re as
>well.
>
>>
>>          # test_build may change the current dir
>>          # get back to root of build dir, before 'touch'
>> diff --git a/tests/Benchmark.Dhrystone/parser.py
>> b/tests/Benchmark.Dhrystone/parser.py
>> index fd07cff..868f9de 100755
>> --- a/tests/Benchmark.Dhrystone/parser.py
>> +++ b/tests/Benchmark.Dhrystone/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>Finally, on all these parser shebangs, I'm not sure what to do.
>
>I experimented with creating a wrapper script, called run_python.sh, that
>determines the correct interpreter to use, and changing these to:
>#!/fuego-core/scripts/run_python.sh
>
>It works, but I don't know how robust it is.  These parser are never invok=
ed
>directly, as far as I can tell.  It might be better to actually remove the=
 shebang
>line to make it explicitly impossible to treat the parser.py scripts as st=
andalone
>programs.
>
>I'm not sure if I'd be breaking anyone's workflow with this or not, so I h=
esitate to
>do that.
>
>What do you think?
In our use cases also we were not directly running them, so we can remove t=
he shebang lines if no one else also not using it directly.

>
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.IOzone/parser.py
>> b/tests/Benchmark.IOzone/parser.py
>> index b1631ae..a56aa0e 100755
>> --- a/tests/Benchmark.IOzone/parser.py
>> +++ b/tests/Benchmark.IOzone/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.Stream/parser.py
>> b/tests/Benchmark.Stream/parser.py
>> index a564d74..926e611 100755
>> --- a/tests/Benchmark.Stream/parser.py
>> +++ b/tests/Benchmark.Stream/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.bonnie/parser.py
>> b/tests/Benchmark.bonnie/parser.py
>> index b68bc3e..962a7cd 100755
>> --- a/tests/Benchmark.bonnie/parser.py
>> +++ b/tests/Benchmark.bonnie/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.cyclictest/parser.py
>> b/tests/Benchmark.cyclictest/parser.py
>> index c29393e..4252654 100755
>> --- a/tests/Benchmark.cyclictest/parser.py
>> +++ b/tests/Benchmark.cyclictest/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>  import os, re, sys
>>  import common as plib
>>
>> diff --git a/tests/Benchmark.fio/parser.py
>> b/tests/Benchmark.fio/parser.py index ab3ea34..fedb734 100644
>> --- a/tests/Benchmark.fio/parser.py
>> +++ b/tests/Benchmark.fio/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>  # See common.py for description of command-line arguments
>>
>>  import os, sys
>> diff --git a/tests/Benchmark.hackbench/parser.py
>> b/tests/Benchmark.hackbench/parser.py
>> index a22c480..3ac9ba5 100755
>> --- a/tests/Benchmark.hackbench/parser.py
>> +++ b/tests/Benchmark.hackbench/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.lmbench2/parser.py
>> b/tests/Benchmark.lmbench2/parser.py
>> index f06f132..410b46e 100755
>> --- a/tests/Benchmark.lmbench2/parser.py
>> +++ b/tests/Benchmark.lmbench2/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.migratetest/parser.py
>> b/tests/Benchmark.migratetest/parser.py
>> index 627ec2d..21b7c56 100755
>> --- a/tests/Benchmark.migratetest/parser.py
>> +++ b/tests/Benchmark.migratetest/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>  import os, re, sys
>>  import common as plib
>>
>> diff --git a/tests/Benchmark.pmqtest/parser.py
>> b/tests/Benchmark.pmqtest/parser.py
>> index 05ee57b..a5c31ba 100755
>> --- a/tests/Benchmark.pmqtest/parser.py
>> +++ b/tests/Benchmark.pmqtest/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.ptsematest/parser.py
>> b/tests/Benchmark.ptsematest/parser.py
>> index 05ee57b..a5c31ba 100755
>> --- a/tests/Benchmark.ptsematest/parser.py
>> +++ b/tests/Benchmark.ptsematest/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.signaltest/parser.py
>> b/tests/Benchmark.signaltest/parser.py
>> index 1992b21..95d4340 100755
>> --- a/tests/Benchmark.signaltest/parser.py
>> +++ b/tests/Benchmark.signaltest/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Benchmark.sigwaittest/parser.py
>> b/tests/Benchmark.sigwaittest/parser.py
>> index 25a0262..120b7b5 100755
>> --- a/tests/Benchmark.sigwaittest/parser.py
>> +++ b/tests/Benchmark.sigwaittest/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>  import os, re, sys
>>  import common as plib
>>
>> diff --git a/tests/Benchmark.svsematest/parser.py
>> b/tests/Benchmark.svsematest/parser.py
>> index 05ee57b..a5c31ba 100755
>> --- a/tests/Benchmark.svsematest/parser.py
>> +++ b/tests/Benchmark.svsematest/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Functional.LTP/fuego_test.sh
>> b/tests/Functional.LTP/fuego_test.sh
>> index ef58bcc..95dea2c 100755
>> --- a/tests/Functional.LTP/fuego_test.sh
>> +++ b/tests/Functional.LTP/fuego_test.sh
>> @@ -475,9 +475,9 @@ function test_processing {
>>          #  ImportError: No module named style
>>          if [ -n "$ORIG_PATH" ] ; then
>>              # Use ORIG_PATH, if defined, so that python works properly
>> -            PATH=3D$ORIG_PATH python ltp_process.py
>> +            PATH=3D$ORIG_PATH python3 ltp_process.py
>>          else
>> -            python ltp_process.py
>> +            python3 ltp_process.py
>>          fi
>>
>>          [ -e results.xlsx ] && cp results.xlsx ${LOGDIR}/results.xlsx
>> diff --git a/tests/Functional.LTP/ltp_process.py
>> b/tests/Functional.LTP/ltp_process.py
>> index 8e2b637..27bc856 100644
>> --- a/tests/Functional.LTP/ltp_process.py
>> +++ b/tests/Functional.LTP/ltp_process.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>  # -*- coding: UTF-8 -*-
>>  from openpyxl import Workbook
>>  from openpyxl.styles import Border, Side, PatternFill, Color,
>> Alignment diff --git a/tests/Functional.LTP/parser.py
>> b/tests/Functional.LTP/parser.py index 9ad7659..3467328 100755
>> --- a/tests/Functional.LTP/parser.py
>> +++ b/tests/Functional.LTP/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>  # -*- coding: UTF-8 -*-
>>  import os, os.path, re, sys
>>  import common as plib
>> diff --git a/tests/Functional.LTP_one_test/parser.py
>> b/tests/Functional.LTP_one_test/parser.py
>> index 312bd5b..7002e35 100755
>> --- a/tests/Functional.LTP_one_test/parser.py
>> +++ b/tests/Functional.LTP_one_test/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>  # See common.py for description of command-line arguments
>>
>>  import os
>> diff --git a/tests/Functional.autopkgtest/parser.py
>> b/tests/Functional.autopkgtest/parser.py
>> index ba3dea1..ba8d2a0 100755
>> --- a/tests/Functional.autopkgtest/parser.py
>> +++ b/tests/Functional.autopkgtest/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/bin/python
>> +#!/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> diff --git a/tests/Functional.linaro/parser.py
>> b/tests/Functional.linaro/parser.py
>> index 48b502b..5bbb5de 100755
>> --- a/tests/Functional.linaro/parser.py
>> +++ b/tests/Functional.linaro/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, sys, collections
>>  import common as plib
>> --
>> 2.20.1
>>
>
>Finally, before changing the shebang in all the parser.py scripts, I'd lik=
e to
>implement the compatibility changes first.  That is, change all the print
>statements to be parenthesized, BEFORE changing the shebang lines.  This w=
ay
>the code is never left in a faulty state (such that a git bisect would fai=
l).
>
>Let me know your thoughts on the issues above.
>
>It looks like at least some of you are on vacation the next few days, but =
I'll wait to
>hear back before I start attacking this problem.
>
>By the way - what distribution of Linux are you testing?  Is this for CIP?=
 or for
>some Toshiba-internal distro of Linux?  (just curious).
We are using on Debian based distribution with Bullseye version, this is on=
e of the reason why we are changing the scripts to support python3, because=
 bullseye in not supporting python2.

At the first we have changed in few tests and some common function scripts =
which we are using, because in our environment the fuego-core runs on test/=
target machine directly this patch works for us, and I am not confident the=
se changes will work with docker-container, so the reason we have not share=
d this patch initially and only the shared some print statement fixes and o=
ther python2to3 compatible fixes.

After studying your suggestion in the above, there are quite few other conv=
ersions required to fully convert to python3.
Let me know if I can do some of the changes.

>
> -- Tim