Message-ID: <20250910142427.61347215@foz.lan>
Date: Wed, 10 Sep 2025 14:24:27 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: Jonathan Corbet <corbet@....net>, Linux Doc Mailing List
 <linux-doc@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 05/19] tools/docs: python_version: move version check
 from sphinx-pre-install

Em Wed, 10 Sep 2025 13:14:33 +0300
Jani Nikula <jani.nikula@...ux.intel.com> escreveu:

> On Thu, 04 Sep 2025, Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:
> > The sphinx-pre-install code has some logic to deal with Python
> > version, which ensures that a minimal version will be enforced
> > for documentation build logic.
> >
> > Move it to a separate library to allow re-using its code.
> >
> > No functional changes.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > ---
> >  tools/docs/lib/python_version.py | 133 +++++++++++++++++++++++++++++++
> >  tools/docs/sphinx-pre-install    | 120 +++-------------------------
> >  2 files changed, 146 insertions(+), 107 deletions(-)
> >  create mode 100644 tools/docs/lib/python_version.py
> >
> > diff --git a/tools/docs/lib/python_version.py b/tools/docs/lib/python_version.py
> > new file mode 100644
> > index 000000000000..0519d524e547
> > --- /dev/null
> > +++ b/tools/docs/lib/python_version.py
> > @@ -0,0 +1,133 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +# Copyright (c) 2017-2025 Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > +
> > +"""
> > +Handle Python version check logic.
> > +
> > +Not all Python versions are supported by scripts. Yet, on some cases,
> > +like during documentation build, a newer version of python could be
> > +available.
> > +
> > +This class allows checking if the minimal requirements are followed.
> > +
> > +Better than that, PythonVersion.check_python() not only checks the minimal
> > +requirements, but it automatically switches to a the newest available
> > +Python version if present.
> > +
> > +"""
> > +
> > +import os
> > +import re
> > +import subprocess
> > +import sys
> > +
> > +from glob import glob
> > +
> > +class PythonVersion:
> > +    """
> > +    Ancillary methods that checks for missing dependencies for different
> > +    types of types, like binaries, python modules, rpm deps, etc.
> > +    """
> > +
> > +    def __init__(self, version):
> > +        """��nitialize self.version tuple from a version string"""
> > +        self.version = self.parse_version(version)
> > +
> > +    @staticmethod
> > +    def parse_version(version):
> > +        """Convert a major.minor.patch version into a tuple"""
> > +        return tuple(int(x) for x in version.split("."))  
> 
> I've written a few quick and dirty semver parsers myself, and it saddens
> me to think of adding a simplistic one in the kernel.
> 
> I'm just wondering, are we doomed to reinventing the wheels in our
> reluctance to depend on anything else?

What do you propose instead, using only internal libs(*)?

In any case, import a library just for one or two single-line
functions seem overkill to me.

(*) As this is used by sphinx-pre-install, which is the script which 
    checks for missing dependencies, whatever we pick, it should not
    use external libs.

> 
> > +
> > +    @staticmethod
> > +    def ver_str(version):
> > +        """Returns a version tuple as major.minor.patch"""
> > +        return ".".join([str(x) for x in version])
> > +
> > +    def __str__(self):
> > +        """Returns a version tuple as major.minor.patch from self.version"""
> > +        return self.ver_str(self.version)
> > +
> > +    @staticmethod
> > +    def get_python_version(cmd):
> > +        """
> > +        Get python version from a Python binary. As we need to detect if
> > +        are out there newer python binaries, we can't rely on sys.release here.
> > +        """
> > +
> > +        kwargs = {}
> > +        if sys.version_info < (3, 7):  
> 
> Checking for things that EOL'd four years ago. Why are we doing this to
> ourselves? Why should we take on maintenance of code that jumps through
> hoops for things that nobody supports anymore?
> 
> In Documentation/process/changes.rst we've declared Python 3.9 as
> minimum, which is also the oldest version supported by upstream (until
> next month). Even Debian oldoldstable (that's two olds) has 3.9.
>
> We're talking about the documentation build. I can undertand being more
> conservative about the kernel build in general, but IMHO this is just
> extra work for absolutely no users out there. And I'm not advocating for
> bleeding edge here.
> 
> We could just throw out a lot of crap by setting newer but still
> moderately concervative required Python (and Sphinx) versions, and bail
> out on older version. Let the user figure out how to get them.
> 
> We don't do this for any other tools either.
> 
> I'm saying that instead of refactoring this overgrown logic to a
> separate file and class, it should be nuked out of the kernel
> completely.

True, but latest SUSE and openSUSE distros (not counting Thumbleweed
rolling version one) still have Python 3.6 as the main version.

They provide 3.9 as well, but the detection script needs to work with
3.6 to discover that.

If we don't have something like that, we should probably return
using the Perl version of sphinx-pre-install script, which is
a lot more compatible with different distros.

> > +            kwargs['universal_newlines'] = True
> > +        else:
> > +            kwargs['text'] = True
> > +
> > +        result = subprocess.run([cmd, "--version"],
> > +                                stdout = subprocess.PIPE,
> > +                                stderr = subprocess.PIPE,
> > +                                **kwargs, check=False)
> > +
> > +        version = result.stdout.strip()
> > +
> > +        match = re.search(r"(\d+\.\d+\.\d+)", version)
> > +        if match:
> > +            return PythonVersion.parse_version(match.group(1))
> > +
> > +        print(f"Can't parse version {version}")
> > +        return (0, 0, 0)
> > +
> > +    @staticmethod
> > +    def find_python(min_version):
> > +        """
> > +        Detect if are out there any python 3.xy version newer than the
> > +        current one.
> > +
> > +        Note: this routine is limited to up to 2 digits for python3. We
> > +        may need to update it one day, hopefully on a distant future.
> > +        """
> > +        patterns = [
> > +            "python3.[0-9]",
> > +            "python3.[0-9][0-9]",
> > +        ]
> > +
> > +        # Seek for a python binary newer than min_version
> > +        for path in os.getenv("PATH", "").split(":"):
> > +            for pattern in patterns:
> > +                for cmd in glob(os.path.join(path, pattern)):
> > +                    if os.path.isfile(cmd) and os.access(cmd, os.X_OK):
> > +                        version = PythonVersion.get_python_version(cmd)
> > +                        if version >= min_version:
> > +                            return cmd
> > +
> > +        return None
> > +
> > +    @staticmethod
> > +    def check_python(min_version):
> > +        """
> > +        Check if the current python binary satisfies our minimal requirement
> > +        for Sphinx build. If not, re-run with a newer version if found.
> > +        """
> > +        cur_ver = sys.version_info[:3]
> > +        if cur_ver >= min_version:
> > +            ver = PythonVersion.ver_str(cur_ver)
> > +            print(f"Python version: {ver}")
> > +
> > +            return
> > +
> > +        python_ver = PythonVersion.ver_str(cur_ver)
> > +
> > +        new_python_cmd = PythonVersion.find_python(min_version)
> > +        if not new_python_cmd:
> > +            print(f"ERROR: Python version {python_ver} is not spported anymore\n")
> > +            print("       Can't find a new version. This script may fail")
> > +            return
> > +
> > +        # Restart script using the newer version  
> 
> I thought the whole idea of restarting was completely rejected by
> approximately everyone?!

This patch is just moving the code. There is a patch after this one
changing the behavior.

> BR,
> Jani.
> 
> 
> > +        script_path = os.path.abspath(sys.argv[0])
> > +        args = [new_python_cmd, script_path] + sys.argv[1:]
> > +
> > +        print(f"Python {python_ver} not supported. Changing to {new_python_cmd}")
> > +
> > +        try:
> > +            os.execv(new_python_cmd, args)
> > +        except OSError as e:
> > +            sys.exit(f"Failed to restart with {new_python_cmd}: {e}")
> > diff --git a/tools/docs/sphinx-pre-install b/tools/docs/sphinx-pre-install
> > index 954ed3dc0645..d6d673b7945c 100755
> > --- a/tools/docs/sphinx-pre-install
> > +++ b/tools/docs/sphinx-pre-install
> > @@ -32,20 +32,10 @@ import subprocess
> >  import sys
> >  from glob import glob
> >  
> > +from lib.python_version import PythonVersion
> >  
> > -def parse_version(version):
> > -    """Convert a major.minor.patch version into a tuple"""
> > -    return tuple(int(x) for x in version.split("."))
> > -
> > -
> > -def ver_str(version):
> > -    """Returns a version tuple as major.minor.patch"""
> > -
> > -    return ".".join([str(x) for x in version])
> > -
> > -
> > -RECOMMENDED_VERSION = parse_version("3.4.3")
> > -MIN_PYTHON_VERSION = parse_version("3.7")
> > +RECOMMENDED_VERSION = PythonVersion("3.4.3").version
> > +MIN_PYTHON_VERSION = PythonVersion("3.7").version
> >  
> >  
> >  class DepManager:
> > @@ -235,95 +225,11 @@ class AncillaryMethods:
> >  
> >          return None
> >  
> > -    @staticmethod
> > -    def get_python_version(cmd):
> > -        """
> > -        Get python version from a Python binary. As we need to detect if
> > -        are out there newer python binaries, we can't rely on sys.release here.
> > -        """
> > -
> > -        result = SphinxDependencyChecker.run([cmd, "--version"],
> > -                                            capture_output=True, text=True)
> > -        version = result.stdout.strip()
> > -
> > -        match = re.search(r"(\d+\.\d+\.\d+)", version)
> > -        if match:
> > -            return parse_version(match.group(1))
> > -
> > -        print(f"Can't parse version {version}")
> > -        return (0, 0, 0)
> > -
> > -    @staticmethod
> > -    def find_python():
> > -        """
> > -        Detect if are out there any python 3.xy version newer than the
> > -        current one.
> > -
> > -        Note: this routine is limited to up to 2 digits for python3. We
> > -        may need to update it one day, hopefully on a distant future.
> > -        """
> > -        patterns = [
> > -            "python3.[0-9]",
> > -            "python3.[0-9][0-9]",
> > -        ]
> > -
> > -        # Seek for a python binary newer than MIN_PYTHON_VERSION
> > -        for path in os.getenv("PATH", "").split(":"):
> > -            for pattern in patterns:
> > -                for cmd in glob(os.path.join(path, pattern)):
> > -                    if os.path.isfile(cmd) and os.access(cmd, os.X_OK):
> > -                        version = SphinxDependencyChecker.get_python_version(cmd)
> > -                        if version >= MIN_PYTHON_VERSION:
> > -                            return cmd
> > -
> > -    @staticmethod
> > -    def check_python():
> > -        """
> > -        Check if the current python binary satisfies our minimal requirement
> > -        for Sphinx build. If not, re-run with a newer version if found.
> > -        """
> > -        cur_ver = sys.version_info[:3]
> > -        if cur_ver >= MIN_PYTHON_VERSION:
> > -            ver = ver_str(cur_ver)
> > -            print(f"Python version: {ver}")
> > -
> > -            # This could be useful for debugging purposes
> > -            if SphinxDependencyChecker.which("docutils"):
> > -                result = SphinxDependencyChecker.run(["docutils", "--version"],
> > -                                                    capture_output=True, text=True)
> > -                ver = result.stdout.strip()
> > -                match = re.search(r"(\d+\.\d+\.\d+)", ver)
> > -                if match:
> > -                    ver = match.group(1)
> > -
> > -                print(f"Docutils version: {ver}")
> > -
> > -            return
> > -
> > -        python_ver = ver_str(cur_ver)
> > -
> > -        new_python_cmd = SphinxDependencyChecker.find_python()
> > -        if not new_python_cmd:
> > -            print(f"ERROR: Python version {python_ver} is not spported anymore\n")
> > -            print("       Can't find a new version. This script may fail")
> > -            return
> > -
> > -        # Restart script using the newer version
> > -        script_path = os.path.abspath(sys.argv[0])
> > -        args = [new_python_cmd, script_path] + sys.argv[1:]
> > -
> > -        print(f"Python {python_ver} not supported. Changing to {new_python_cmd}")
> > -
> > -        try:
> > -            os.execv(new_python_cmd, args)
> > -        except OSError as e:
> > -            sys.exit(f"Failed to restart with {new_python_cmd}: {e}")
> > -
> >      @staticmethod
> >      def run(*args, **kwargs):
> >          """
> >          Excecute a command, hiding its output by default.
> > -        Preserve comatibility with older Python versions.
> > +        Preserve compatibility with older Python versions.
> >          """
> >  
> >          capture_output = kwargs.pop('capture_output', False)
> > @@ -527,11 +433,11 @@ class MissingCheckers(AncillaryMethods):
> >          for line in result.stdout.split("\n"):
> >              match = re.match(r"^sphinx-build\s+([\d\.]+)(?:\+(?:/[\da-f]+)|b\d+)?\s*$", line)
> >              if match:
> > -                return parse_version(match.group(1))
> > +                return PythonVersion.parse_version(match.group(1))
> >  
> >              match = re.match(r"^Sphinx.*\s+([\d\.]+)\s*$", line)
> >              if match:
> > -                return parse_version(match.group(1))
> > +                return PythonVersion.parse_version(match.group(1))
> >  
> >      def check_sphinx(self, conf):
> >          """
> > @@ -542,7 +448,7 @@ class MissingCheckers(AncillaryMethods):
> >                  for line in f:
> >                      match = re.match(r"^\s*needs_sphinx\s*=\s*[\'\"]([\d\.]+)[\'\"]", line)
> >                      if match:
> > -                        self.min_version = parse_version(match.group(1))
> > +                        self.min_version = PythonVersion.parse_version(match.group(1))
> >                          break
> >          except IOError:
> >              sys.exit(f"Can't open {conf}")
> > @@ -562,8 +468,8 @@ class MissingCheckers(AncillaryMethods):
> >              sys.exit(f"{sphinx} didn't return its version")
> >  
> >          if self.cur_version < self.min_version:
> > -            curver = ver_str(self.cur_version)
> > -            minver = ver_str(self.min_version)
> > +            curver = PythonVersion.ver_str(self.cur_version)
> > +            minver = PythonVersion.ver_str(self.min_version)
> >  
> >              print(f"ERROR: Sphinx version is {curver}. It should be >= {minver}")
> >              self.need_sphinx = 1
> > @@ -1304,7 +1210,7 @@ class SphinxDependencyChecker(MissingCheckers):
> >              else:
> >                  if self.need_sphinx and ver >= self.min_version:
> >                      return (f, ver)
> > -                elif parse_version(ver) > self.cur_version:
> > +                elif PythonVersion.parse_version(ver) > self.cur_version:
> >                      return (f, ver)
> >  
> >          return ("", ver)
> > @@ -1411,7 +1317,7 @@ class SphinxDependencyChecker(MissingCheckers):
> >              return
> >  
> >          if self.latest_avail_ver:
> > -            latest_avail_ver = ver_str(self.latest_avail_ver)
> > +            latest_avail_ver = PythonVersion.ver_str(self.latest_avail_ver)
> >  
> >          if not self.need_sphinx:
> >              # sphinx-build is present and its version is >= $min_version
> > @@ -1507,7 +1413,7 @@ class SphinxDependencyChecker(MissingCheckers):
> >          else:
> >              print("Unknown OS")
> >          if self.cur_version != (0, 0, 0):
> > -            ver = ver_str(self.cur_version)
> > +            ver = PythonVersion.ver_str(self.cur_version)
> >              print(f"Sphinx version: {ver}\n")
> >  
> >          # Check the type of virtual env, depending on Python version
> > @@ -1613,7 +1519,7 @@ def main():
> >  
> >      checker = SphinxDependencyChecker(args)
> >  
> > -    checker.check_python()
> > +    PythonVersion.check_python(MIN_PYTHON_VERSION)
> >      checker.check_needs()
> >  
> >  # Call main if not used as module  
> 



Thanks,
Mauro

Powered by blists - more mailing lists