Message-ID: <Yv8yRv2BMVgmb/n+@leoy-yangtze.lan>
Date:   Fri, 19 Aug 2022 14:48:38 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Chuck Zmudzinski <brchuckz@...scape.net>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alan Bartlett <ajb@...epo.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        linux-perf-users@...r.kernel.org, ElRepo <contact@...epo.org>,
        Akemi Yagi <toracat@...epo.org>, Jiri Olsa <jolsa@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf scripts python: Let script to be python2 compliant

On Thu, Aug 18, 2022 at 07:52:22AM -0700, Ian Rogers wrote:

[...]

> > > I see this has already been committed. I agree it should not
> > > stay in the kernel tree for long. At some point in the future
> > > it will most likely cause problems if it is not reverted.
> >
> > It is a bit confused me that here actually we are worrying about the
> > python distro issue, e.g. python2 is obsolete, so it's risky to keep
> > using python2 in the system, especially if python2 has potential
> > security issue.  So the system (e.g. RHEL7) needs to upgrade its python
> > distro from python2 to python3.
> >
> > But this doesn't mean the python script cannot be compatible for both
> > python2 and python3.  I verified this patch, it should can work for
> > both python2 and python3 on my PC.
> >
> > Another concern is there have some python scripts in perf, I think most
> > of them are python2 compatible, should we update all of them to be only
> > python3 compatible?
> 
> I think there are a lot of things that need to be done for python in
> the perf build. For example, perf script is using deprecated python C
> APIs. . As you say string formatting isn't the biggest issue. What I
> think we need to do is set a minimum bar for what is supported, for
> jevents.py that is python 3.6.
> 
> The problem with python2 compatibility can be seen with this:
> https://lore.kernel.org/all/20220615014206.26651-1-irogers@google.com/
> python3 is deprecating APIs which are the only API choices on python2.
> So we can:
> 1) have 2 scripts, one for python2 and one for python3, possibly
> varying by release of python3 depending on when a deprecated API is
> removed
> 2) always be stuck on python 2 as our lowest bar for compatibility
> 3) forget about python 2, set compatibility at some reasonably but not
> totally new version of python like 3.6 and move this forward inline
> with supported versions by the python community
> 
> I favor (3) not least as testing (1) is a challenge/chore and if
> something is wrong with python2, well it is on us. I think we
> shouldn't aim to support more python than what the python community
> itself aims to support. They are clear that python 2 is dead.

I agree that option 3 is the good way to move forward.

Thanks a lot for detailed explaination.

> Going back to f-strings, they are considered the more pythonic way to
> write things and make it easier to read code, eye-ball mistakes, etc.
> We want to have the best code possible in the perf codebase and so
> changes to use f-strings to the python scripts in perf should be
> welcomed - RHEL7 customers will just have to get with the program imo,
> well they should upgrade off of RHEL7. Python 3.6 is hardly new and
> this causes issues in jevents.py, for example, as I can't rely on the
> string removesuffix function being available (added in python 3.9).
> 
> There is a bunch of clean up necessary to get rid of python 2 from the
> build, and we took a step in this direction by defaulting to python 3
> (not without pain) in:
> https://lore.kernel.org/all/20220616044806.47770-2-irogers@google.com/
> 
> Should we make all the scripts python 3 only? It comes down to people
> writing patches. If someone updates a script and uses an f-string, I
> don't think we should code review and reject the patch. Should we
> proactively go and clean up python code in perf to make it more
> pythonic? Sounds good to me, and I might suggest a list of other clean
> ups we need to do given you have the time :-)

It's fine for me to revert this patch in an appropriate time.

Regard to other python scripts in Perf, seems to me a pragmatic method is
that as you said we can set a bar for python version (e.g. 3.6), then we
can go through python scripts one by one to test if any script is not
python 3.6 compatiable or not.  If not, then we can commit patch for
it.

"the more pythonic way" is a subjective words? :)  I am lack the
knowledge to judge if any things are more "pythonic" and can replace with
the existed code.  But I think it would be a good start to use f-string
to replace old print sentences.  When I have some free time, I will come
back to check with you and other maintainers if I can help a bit for this.

Thanks,
Leo

Powered by blists - more mailing lists