Message-ID: <CAP-5=fX8OVBFGXAgshstMBMFo4ULvMBdZnE_2h-RsamJ6q1qow@mail.gmail.com>
Date:   Thu, 18 Aug 2022 07:52:22 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Leo Yan <leo.yan@...aro.org>
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 Wed, Aug 17, 2022 at 8:03 PM Leo Yan <leo.yan@...aro.org> wrote:
>
> On Wed, Aug 17, 2022 at 09:12:24PM -0400, Chuck Zmudzinski wrote:
>
> [...]
>
> > > > > > > Someone reported a problem in a system they used, the author of the code
> > > > > > > in question posted a patch allowing perf to be used in such old systems,
> > > > > > > doesn't get in the way of newer systems, small patch, merged, life goes
> > > > > > > on.
> > > > >
> > > > > Considering the proposed patch, can you be sure that replacing the
> > > > > f-string format with the legacy format won't cause a regression for
> > > > > some python3 user somewhere when this hits the real world? Even
> > > > > if it does not cause a regression today, as new versions and features
> > > > > are added to python3, can you be sure none of those new features
> > > > > will depend on the upgrade from the legacy format to the f-string
> > > > > format here to work properly? So many regressions happen because
> > > > > the people who write patches cannot possibly foresee how their
> > > > > patch is going to affect the millions of Linux users out there, but still
> > > > > they are certain it will not cause a regression somewhere. So how
> > > > > can the chances that this patch will cause a regression be minimized?
> > > > >
> > > > > It seems to me for this to be suitable for the Linux kernel, the
> > > > > default should be to use the modern python3 format and only
> > > > > enable python2 compatibility via a sysctl setting and/or kernel boot
> > > > > option for those who are still using python2. There should be no
> > > > > change to the behavior of the kernel for users who have upgraded
> > > > > to python3. But I don't see any such consideration for python3
> > > > > users in this patch.
> > > >
> > > > Sorry, I didn't see this is a script, LOL! So obviously a sysctl or boot option
> > > > does not apply. But can't the script implement this simple logic:
> > > >
> > > > If python version = 3 use f-string format
> > > > if python version = 2 use traditional string format
> > >
> > > Doing this in the script would be noisy, having two scripts less than
> > > ideal. I'd suggest we wait two weeks, declare the official death of
> > > RHEL7 without "rpm -i python3" and then revert the python3 to python2
> > > patch. There are plenty of things to worry about and python2 shouldn't
> > > be one of them (it died over 2 years ago).
> >
> > 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.

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 :-)

Thanks,
Ian

> Thanks,
> Leo

Powered by blists - more mailing lists