[PATCH 0/8] Sphinx extension fix + logging/warning cleanups

[PATCH 0/8] Sphinx extension fix + logging/warning cleanups

[Vegard Nossum]

Hi,

The first patch in the series fixes the "UnboundLocalError: local
variable 'fname' referenced before assignment" error and should
probably go into v6.8 and then stable.

The rest are just cleanup/improvements, mainly the removal of
kernellog.py in favour of sphinx.util.logging, but also adding a
warning if you pass something strange to the kernel-abi:: or
kernel-feat:: directives.

The reason I'm sending these two things together is that there
is a very slight dependency on the very first patch.

I've tested on 2.4.4, 4.3.2, and 7.3.0+/b660154eaf71 by running
make cleandocs; make htmldocs. Figures, features, and ABI lists
looked fine to me.


Vegard

---

Vegard Nossum (8):
  docs: kernel_feat.py: fix build error for missing files
  docs: kernel_{abi,feat}.py: use doc.current_source
  doc: kernel_abi.py: convert to sphinx.util.logging
  doc: kernel_feat.py: convert to sphinx.util.logging
  doc: kerneldoc.py: convert to sphinx.util.logging
  doc: kfigure.py: convert to sphinx.util.logging
  doc: remove kernellog.py
  doc: kernel_{abi,feat}.py: warn about missing directory

 Documentation/sphinx/kernel_abi.py  | 15 +++++--
 Documentation/sphinx/kernel_feat.py | 19 +++++----
 Documentation/sphinx/kerneldoc.py   | 14 +++---
 Documentation/sphinx/kernellog.py   | 22 ----------
 Documentation/sphinx/kfigure.py     | 66 ++++++++++++++---------------
 5 files changed, 60 insertions(+), 76 deletions(-)
 delete mode 100644 Documentation/sphinx/kernellog.py

-- 
2.34.1

[PATCH 1/8] docs: kernel_feat.py: fix build error for missing files

[Vegard Nossum]

If the directory passed to the '.. kernel-feat::' directive does not
exist or the get_feat.pl script does not find any files to extract
features from, Sphinx will report the following error:

    Sphinx parallel build error:
    UnboundLocalError: local variable 'fname' referenced before assignment
    make[2]: *** [Documentation/Makefile:102: htmldocs] Error 2

This is due to how I changed the script in c48a7c44a1d0 ("docs:
kernel_feat.py: fix potential command injection"). Before that, the
filename passed along to self.nestedParse() in this case was weirdly
just the whole get_feat.pl invocation.

We can fix it by doing what kernel_abi.py does -- just pass
self.arguments[0] as 'fname'.

Fixes: c48a7c44a1d0 ("docs: kernel_feat.py: fix potential command injection")
Cc: Justin Forbes <jforbes@fedoraproject.org>
Cc: Salvatore Bonaccorso <carnil@debian.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 Documentation/sphinx/kernel_feat.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
index b9df61eb4501..03ace5f01b5c 100644
--- a/Documentation/sphinx/kernel_feat.py
+++ b/Documentation/sphinx/kernel_feat.py
@@ -109,7 +109,7 @@ class KernelFeat(Directive):
             else:
                 out_lines += line + "\n"
 
-        nodeList = self.nestedParse(out_lines, fname)
+        nodeList = self.nestedParse(out_lines, self.arguments[0])
         return nodeList
 
     def nestedParse(self, lines, fname):
-- 
2.34.1

[PATCH 2/8] docs: kernel_{abi,feat}.py: use doc.current_source

[Vegard Nossum]

It probably doesn't matter a whole lot what we actually pass here,
but the .rst being processed seems most appropriate to me.

This presumably gets used by Shpinx to record/report where each line
of .rst source originates.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 Documentation/sphinx/kernel_abi.py  | 2 +-
 Documentation/sphinx/kernel_feat.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/sphinx/kernel_abi.py b/Documentation/sphinx/kernel_abi.py
index 5911bd0d7965..288f26097569 100644
--- a/Documentation/sphinx/kernel_abi.py
+++ b/Documentation/sphinx/kernel_abi.py
@@ -88,7 +88,7 @@ class KernelCmd(Directive):
             args.append('--rst-source')
 
         lines = subprocess.check_output(args, cwd=os.path.dirname(doc.current_source)).decode('utf-8')
-        nodeList = self.nestedParse(lines, self.arguments[0])
+        nodeList = self.nestedParse(lines, doc.current_source)
         return nodeList
 
     def nestedParse(self, lines, fname):
diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
index 03ace5f01b5c..3493621d1a4e 100644
--- a/Documentation/sphinx/kernel_feat.py
+++ b/Documentation/sphinx/kernel_feat.py
@@ -109,7 +109,7 @@ class KernelFeat(Directive):
             else:
                 out_lines += line + "\n"
 
-        nodeList = self.nestedParse(out_lines, self.arguments[0])
+        nodeList = self.nestedParse(out_lines, doc.current_source)
         return nodeList
 
     def nestedParse(self, lines, fname):
-- 
2.34.1

[PATCH 3/8] doc: kernel_abi.py: convert to sphinx.util.logging

[Vegard Nossum]

As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
to 2.4.4"), we can use Sphinx's built-in logging facilities.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 Documentation/sphinx/kernel_abi.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/sphinx/kernel_abi.py b/Documentation/sphinx/kernel_abi.py
index 288f26097569..9eb7282cc941 100644
--- a/Documentation/sphinx/kernel_abi.py
+++ b/Documentation/sphinx/kernel_abi.py
@@ -37,16 +37,18 @@ import os
 import subprocess
 import sys
 import re
-import kernellog
 
 from docutils import nodes, statemachine
 from docutils.statemachine import ViewList
 from docutils.parsers.rst import directives, Directive
 from docutils.utils.error_reporting import ErrorString
+from sphinx.util import logging
 from sphinx.util.docutils import switch_source_input
 
 __version__  = '1.0'
 
+logger = logging.getLogger(__name__)
+
 def setup(app):
 
     app.add_directive("kernel-abi", KernelCmd)
@@ -129,7 +131,7 @@ class KernelCmd(Directive):
             else:
                 content.append(line, f, ln)
 
-        kernellog.info(self.state.document.settings.env.app, "%s: parsed %i lines" % (fname, n))
+        logger.info("%s: parsed %i lines", fname, n)
 
         if content:
             self.do_parse(content, node)
-- 
2.34.1

[PATCH 4/8] doc: kernel_feat.py: convert to sphinx.util.logging

[Vegard Nossum]

As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
to 2.4.4"), we can use Sphinx's built-in logging facilities.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 Documentation/sphinx/kernel_feat.py | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
index 3493621d1a4e..f1c9e4a54964 100644
--- a/Documentation/sphinx/kernel_feat.py
+++ b/Documentation/sphinx/kernel_feat.py
@@ -41,10 +41,13 @@ from docutils import nodes, statemachine
 from docutils.statemachine import ViewList
 from docutils.parsers.rst import directives, Directive
 from docutils.utils.error_reporting import ErrorString
+from sphinx.util import logging
 from sphinx.util.docutils import switch_source_input
 
 __version__  = '1.0'
 
+logger = logging.getLogger(__name__)
+
 def setup(app):
 
     app.add_directive("kernel-feat", KernelFeat)
@@ -67,12 +70,6 @@ class KernelFeat(Directive):
         "debug"     : directives.flag
     }
 
-    def warn(self, message, **replace):
-        replace["fname"]   = self.state.document.current_source
-        replace["line_no"] = replace.get("line_no", self.lineno)
-        message = ("%(fname)s:%(line_no)s: [kernel-feat WARN] : " + message) % replace
-        self.state.document.settings.env.app.warn(message, prefix="")
-
     def run(self):
         doc = self.state.document
         if not doc.settings.file_insertion_enabled:
-- 
2.34.1

[PATCH 5/8] doc: kerneldoc.py: convert to sphinx.util.logging

[Vegard Nossum]

As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
to 2.4.4"), we can use Sphinx's built-in logging facilities.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 Documentation/sphinx/kerneldoc.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
index 7acf09963daa..c74a3e417cea 100644
--- a/Documentation/sphinx/kerneldoc.py
+++ b/Documentation/sphinx/kerneldoc.py
@@ -38,11 +38,13 @@ from docutils import nodes, statemachine
 from docutils.statemachine import ViewList
 from docutils.parsers.rst import directives, Directive
 import sphinx
+from sphinx.util import logging
 from sphinx.util.docutils import switch_source_input
-import kernellog
 
 __version__  = '1.0'
 
+logger = logging.getLogger(__name__)
+
 class KernelDocDirective(Directive):
     """Extract kernel-doc comments from the specified file"""
     required_argument = 1
@@ -109,8 +111,7 @@ class KernelDocDirective(Directive):
         cmd += [filename]
 
         try:
-            kernellog.verbose(env.app,
-                              'calling kernel-doc \'%s\'' % (" ".join(cmd)))
+            logger.verbose('calling kernel-doc \'%s\'', " ".join(cmd))
 
             p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
             out, err = p.communicate()
@@ -120,8 +121,7 @@ class KernelDocDirective(Directive):
             if p.returncode != 0:
                 sys.stderr.write(err)
 
-                kernellog.warn(env.app,
-                               'kernel-doc \'%s\' failed with return code %d' % (" ".join(cmd), p.returncode))
+                logger.warning('kernel-doc \'%s\' failed with return code %d', " ".join(cmd), p.returncode)
                 return [nodes.error(None, nodes.paragraph(text = "kernel-doc missing"))]
             elif env.config.kerneldoc_verbosity > 0:
                 sys.stderr.write(err)
@@ -148,8 +148,8 @@ class KernelDocDirective(Directive):
             return node.children
 
         except Exception as e:  # pylint: disable=W0703
-            kernellog.warn(env.app, 'kernel-doc \'%s\' processing failed with: %s' %
-                           (" ".join(cmd), str(e)))
+            logger.warning('kernel-doc \'%s\' processing failed with: %s',
+                           " ".join(cmd), str(e))
             return [nodes.error(None, nodes.paragraph(text = "kernel-doc missing"))]
 
     def do_parse(self, result, node):
-- 
2.34.1

[PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Vegard Nossum]

As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
to 2.4.4"), we can use Sphinx's built-in logging facilities.

Gotchas:
- remove first argument 'app' from all calls
- instead of (fmt % (args)), use (fmt, args)
- instead of ("<fmt>: " + str) use ("<fmt: %s>", str)
- realign wrapped lines

I changed the "Neither inkscape(1) nor convert(1) found." message from a
.verbose() to a .warning(), since that actually affects the output in a
big way.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 Documentation/sphinx/kfigure.py | 66 ++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/Documentation/sphinx/kfigure.py b/Documentation/sphinx/kfigure.py
index 97166333b727..b58f6458af63 100644
--- a/Documentation/sphinx/kfigure.py
+++ b/Documentation/sphinx/kfigure.py
@@ -58,13 +58,15 @@ from docutils.statemachine import ViewList
 from docutils.parsers.rst import directives
 from docutils.parsers.rst.directives import images
 import sphinx
+from sphinx.util import logging
 from sphinx.util.nodes import clean_astext
-import kernellog
 
 Figure = images.Figure
 
 __version__  = '1.0.0'
 
+logger = logging.getLogger(__name__)
+
 # simple helper
 # -------------
 
@@ -170,7 +172,7 @@ def setupTools(app):
     """
     global dot_cmd, dot_Tpdf, convert_cmd, rsvg_convert_cmd   # pylint: disable=W0603
     global inkscape_cmd, inkscape_ver_one  # pylint: disable=W0603
-    kernellog.verbose(app, "kfigure: check installed tools ...")
+    logger.verbose("kfigure: check installed tools ...")
 
     dot_cmd = which('dot')
     convert_cmd = which('convert')
@@ -178,7 +180,7 @@ def setupTools(app):
     inkscape_cmd = which('inkscape')
 
     if dot_cmd:
-        kernellog.verbose(app, "use dot(1) from: " + dot_cmd)
+        logger.verbose("use dot(1) from: %s", dot_cmd)
 
         try:
             dot_Thelp_list = subprocess.check_output([dot_cmd, '-Thelp'],
@@ -190,10 +192,10 @@ def setupTools(app):
         dot_Tpdf_ptn = b'pdf'
         dot_Tpdf = re.search(dot_Tpdf_ptn, dot_Thelp_list)
     else:
-        kernellog.warn(app, "dot(1) not found, for better output quality install "
+        logger.warning("dot(1) not found, for better output quality install "
                        "graphviz from https://www.graphviz.org")
     if inkscape_cmd:
-        kernellog.verbose(app, "use inkscape(1) from: " + inkscape_cmd)
+        logger.verbose("use inkscape(1) from: %s", inkscape_cmd)
         inkscape_ver = subprocess.check_output([inkscape_cmd, '--version'],
                                                stderr=subprocess.DEVNULL)
         ver_one_ptn = b'Inkscape 1'
@@ -204,26 +206,26 @@ def setupTools(app):
 
     else:
         if convert_cmd:
-            kernellog.verbose(app, "use convert(1) from: " + convert_cmd)
+            logger.verbose("use convert(1) from: %s", convert_cmd)
         else:
-            kernellog.verbose(app,
+            logger.warning(
                 "Neither inkscape(1) nor convert(1) found.\n"
                 "For SVG to PDF conversion, "
                 "install either Inkscape (https://inkscape.org/) (preferred) or\n"
                 "ImageMagick (https://www.imagemagick.org)")
 
         if rsvg_convert_cmd:
-            kernellog.verbose(app, "use rsvg-convert(1) from: " + rsvg_convert_cmd)
-            kernellog.verbose(app, "use 'dot -Tsvg' and rsvg-convert(1) for DOT -> PDF conversion")
+            logger.verbose("use rsvg-convert(1) from: %s", rsvg_convert_cmd)
+            logger.verbose("use 'dot -Tsvg' and rsvg-convert(1) for DOT -> PDF conversion")
             dot_Tpdf = False
         else:
-            kernellog.verbose(app,
+            logger.verbose(
                 "rsvg-convert(1) not found.\n"
                 "  SVG rendering of convert(1) is done by ImageMagick-native renderer.")
             if dot_Tpdf:
-                kernellog.verbose(app, "use 'dot -Tpdf' for DOT -> PDF conversion")
+                logger.verbose("use 'dot -Tpdf' for DOT -> PDF conversion")
             else:
-                kernellog.verbose(app, "use 'dot -Tsvg' and convert(1) for DOT -> PDF conversion")
+                logger.verbose("use 'dot -Tsvg' and convert(1) for DOT -> PDF conversion")
 
 
 # integrate conversion tools
@@ -257,13 +259,12 @@ def convert_image(img_node, translator, src_fname=None):
 
     # in kernel builds, use 'make SPHINXOPTS=-v' to see verbose messages
 
-    kernellog.verbose(app, 'assert best format for: ' + img_node['uri'])
+    logger.verbose('assert best format for: %s', img_node['uri'])
 
     if in_ext == '.dot':
 
         if not dot_cmd:
-            kernellog.verbose(app,
-                              "dot from graphviz not available / include DOT raw.")
+            logger.verbose("dot from graphviz not available / include DOT raw.")
             img_node.replace_self(file2literal(src_fname))
 
         elif translator.builder.format == 'latex':
@@ -290,10 +291,9 @@ def convert_image(img_node, translator, src_fname=None):
 
         if translator.builder.format == 'latex':
             if not inkscape_cmd and convert_cmd is None:
-                kernellog.warn(app,
-                                  "no SVG to PDF conversion available / include SVG raw."
-                                  "\nIncluding large raw SVGs can cause xelatex error."
-                                  "\nInstall Inkscape (preferred) or ImageMagick.")
+                logger.warning("no SVG to PDF conversion available / include SVG raw.\n"
+                               "Including large raw SVGs can cause xelatex error.\n"
+                               "Install Inkscape (preferred) or ImageMagick.")
                 img_node.replace_self(file2literal(src_fname))
             else:
                 dst_fname = path.join(translator.builder.outdir, fname + '.pdf')
@@ -306,15 +306,14 @@ def convert_image(img_node, translator, src_fname=None):
         _name = dst_fname[len(str(translator.builder.outdir)) + 1:]
 
         if isNewer(dst_fname, src_fname):
-            kernellog.verbose(app,
-                              "convert: {out}/%s already exists and is newer" % _name)
+            logger.verbose("convert: {out}/%s already exists and is newer" % _name)
 
         else:
             ok = False
             mkdir(path.dirname(dst_fname))
 
             if in_ext == '.dot':
-                kernellog.verbose(app, 'convert DOT to: {out}/' + _name)
+                logger.verbose('convert DOT to: {out}/%s', _name)
                 if translator.builder.format == 'latex' and not dot_Tpdf:
                     svg_fname = path.join(translator.builder.outdir, fname + '.svg')
                     ok1 = dot2format(app, src_fname, svg_fname)
@@ -325,7 +324,7 @@ def convert_image(img_node, translator, src_fname=None):
                     ok = dot2format(app, src_fname, dst_fname)
 
             elif in_ext == '.svg':
-                kernellog.verbose(app, 'convert SVG to: {out}/' + _name)
+                logger.verbose('convert SVG to: {out}/%s', _name)
                 ok = svg2pdf(app, src_fname, dst_fname)
 
             if not ok:
@@ -354,8 +353,7 @@ def dot2format(app, dot_fname, out_fname):
     with open(out_fname, "w") as out:
         exit_code = subprocess.call(cmd, stdout = out)
         if exit_code != 0:
-            kernellog.warn(app,
-                          "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
+            logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
     return bool(exit_code == 0)
 
 def svg2pdf(app, svg_fname, pdf_fname):
@@ -388,13 +386,13 @@ def svg2pdf(app, svg_fname, pdf_fname):
         pass
 
     if exit_code != 0:
-        kernellog.warn(app, "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
+        logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
         if warning_msg:
-            kernellog.warn(app, "Warning msg from %s: %s"
-                           % (cmd_name, str(warning_msg, 'utf-8')))
+            logger.warning("Warning msg from %s: %s",
+                           cmd_name, str(warning_msg, 'utf-8'))
     elif warning_msg:
-        kernellog.verbose(app, "Warning msg from %s (likely harmless):\n%s"
-                          % (cmd_name, str(warning_msg, 'utf-8')))
+        logger.warning("Warning msg from %s (likely harmless):\n%s",
+                       cmd_name, str(warning_msg, 'utf-8'))
 
     return bool(exit_code == 0)
 
@@ -418,7 +416,7 @@ def svg2pdf_by_rsvg(app, svg_fname, pdf_fname):
         # use stdout and stderr from parent
         exit_code = subprocess.call(cmd)
         if exit_code != 0:
-            kernellog.warn(app, "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
+            logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
         ok = bool(exit_code == 0)
 
     return ok
@@ -513,15 +511,15 @@ def visit_kernel_render(self, node):
     app = self.builder.app
     srclang = node.get('srclang')
 
-    kernellog.verbose(app, 'visit kernel-render node lang: "%s"' % (srclang))
+    logger.verbose('visit kernel-render node lang: "%s"', srclang)
 
     tmp_ext = RENDER_MARKUP_EXT.get(srclang, None)
     if tmp_ext is None:
-        kernellog.warn(app, 'kernel-render: "%s" unknown / include raw.' % (srclang))
+        logger.warning('kernel-render: "%s" unknown / include raw.', srclang)
         return
 
     if not dot_cmd and tmp_ext == '.dot':
-        kernellog.verbose(app, "dot from graphviz not available / include raw.")
+        logger.verbose("dot from graphviz not available / include raw.")
         return
 
     literal_block = node[0]
-- 
2.34.1

[PATCH 7/8] doc: remove kernellog.py

[Vegard Nossum]

With the last kernellog users gone, we can remove this compatibility
layer.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 Documentation/sphinx/kernellog.py | 22 ----------------------
 1 file changed, 22 deletions(-)
 delete mode 100644 Documentation/sphinx/kernellog.py

diff --git a/Documentation/sphinx/kernellog.py b/Documentation/sphinx/kernellog.py
deleted file mode 100644
index 0bc00c138cad..000000000000
--- a/Documentation/sphinx/kernellog.py
+++ /dev/null
@@ -1,22 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0
-#
-# Sphinx has deprecated its older logging interface, but the replacement
-# only goes back to 1.6.  So here's a wrapper layer to keep around for
-# as long as we support 1.4.
-#
-# We don't support 1.4 anymore, but we'll keep the wrappers around until
-# we change all the code to not use them anymore :)
-#
-import sphinx
-from sphinx.util import logging
-
-logger = logging.getLogger('kerneldoc')
-
-def warn(app, message):
-    logger.warning(message)
-
-def verbose(app, message):
-    logger.verbose(message)
-
-def info(app, message):
-    logger.info(message)
-- 
2.34.1

[PATCH 8/8] doc: kernel_{abi,feat}.py: warn about missing directory

[Vegard Nossum]

The underlying perl script get_feat.pl does not complain when you pass
nonexistent directories, and just produces empty/useless output:

    $ scripts/get_feat.pl rest --enable-fname --dir asdfasdf --arch arm64
    ====================================
    Feature status on arm64 architecture
    ====================================

    =========  =======  =======  ======  ===========
    Subsystem  Feature  Kconfig  Status  Description
    =========  =======  =======  ======  ===========
    =========  =======  =======  ======  ===========

Let's add an early sanity check and a warning if the check fails.

get_abi.pl doesn't have exactly the same issue as it actually produces
an error, but we can add the same check for consistency.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 Documentation/sphinx/kernel_abi.py  | 7 ++++++-
 Documentation/sphinx/kernel_feat.py | 8 ++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/sphinx/kernel_abi.py b/Documentation/sphinx/kernel_abi.py
index 9eb7282cc941..52af2750e634 100644
--- a/Documentation/sphinx/kernel_abi.py
+++ b/Documentation/sphinx/kernel_abi.py
@@ -79,11 +79,16 @@ class KernelCmd(Directive):
 
         srctree = os.path.abspath(os.environ["srctree"])
 
+        dir_path = os.path.join(srctree, 'Documentation', self.arguments[0])
+        if not os.path.exists(dir_path):
+            logger.warning("path does not exist: '%s'", dir_path,
+                           location=(self.state.document.current_source, self.lineno))
+
         args = [
             os.path.join(srctree, 'scripts/get_abi.pl'),
             'rest',
             '--enable-lineno',
-            '--dir', os.path.join(srctree, 'Documentation', self.arguments[0]),
+            '--dir', dir_path,
         ]
 
         if 'rst' in self.options:
diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
index f1c9e4a54964..e0bc6e03579c 100644
--- a/Documentation/sphinx/kernel_feat.py
+++ b/Documentation/sphinx/kernel_feat.py
@@ -79,12 +79,16 @@ class KernelFeat(Directive):
 
         srctree = os.path.abspath(os.environ["srctree"])
 
+        dir_path = os.path.join(srctree, 'Documentation', self.arguments[0])
+        if not os.path.exists(dir_path):
+            logger.warning("path does not exist: '%s'", dir_path,
+                           location=(self.state.document.current_source, self.lineno))
+
         args = [
             os.path.join(srctree, 'scripts/get_feat.pl'),
             'rest',
             '--enable-fname',
-            '--dir',
-            os.path.join(srctree, 'Documentation', self.arguments[0]),
+            '--dir', dir_path,
         ]
 
         if len(self.arguments) > 1:
-- 
2.34.1

Re: [PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Akira Yokosawa]

On Mon,  5 Feb 2024 18:51:31 +0100, Vegard Nossum wrote:
> As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
> to 2.4.4"), we can use Sphinx's built-in logging facilities.
> 
> Gotchas:
> - remove first argument 'app' from all calls
> - instead of (fmt % (args)), use (fmt, args)
> - instead of ("<fmt>: " + str) use ("<fmt: %s>", str)
> - realign wrapped lines
> 
> I changed the "Neither inkscape(1) nor convert(1) found." message from a
> .verbose() to a .warning(), since that actually affects the output in a
> big way.

No, please don't!

you are partially reverting commit d987d5ae51ec ("docs: kfigure.py:
Don't warn of missing PDF converter in 'make htmldocs'").

See its changelog for why it should be kept verbase.

        Thanks, Akira

> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/sphinx/kfigure.py | 66 ++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 34 deletions(-)

Re: [PATCH 1/8] docs: kernel_feat.py: fix build error for missing files

[Mauro Carvalho Chehab]

Em Mon,  5 Feb 2024 18:51:26 +0100
Vegard Nossum <vegard.nossum@oracle.com> escreveu:

> If the directory passed to the '.. kernel-feat::' directive does not
> exist or the get_feat.pl script does not find any files to extract
> features from, Sphinx will report the following error:
> 
>     Sphinx parallel build error:
>     UnboundLocalError: local variable 'fname' referenced before assignment
>     make[2]: *** [Documentation/Makefile:102: htmldocs] Error 2
> 
> This is due to how I changed the script in c48a7c44a1d0 ("docs:
> kernel_feat.py: fix potential command injection"). Before that, the
> filename passed along to self.nestedParse() in this case was weirdly
> just the whole get_feat.pl invocation.
> 
> We can fix it by doing what kernel_abi.py does -- just pass
> self.arguments[0] as 'fname'.
> 
> Fixes: c48a7c44a1d0 ("docs: kernel_feat.py: fix potential command injection")
> Cc: Justin Forbes <jforbes@fedoraproject.org>
> Cc: Salvatore Bonaccorso <carnil@debian.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  Documentation/sphinx/kernel_feat.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
> index b9df61eb4501..03ace5f01b5c 100644
> --- a/Documentation/sphinx/kernel_feat.py
> +++ b/Documentation/sphinx/kernel_feat.py
> @@ -109,7 +109,7 @@ class KernelFeat(Directive):
>              else:
>                  out_lines += line + "\n"
>  
> -        nodeList = self.nestedParse(out_lines, fname)
> +        nodeList = self.nestedParse(out_lines, self.arguments[0])
>          return nodeList
>  
>      def nestedParse(self, lines, fname):



Thanks,
Mauro

Re: [PATCH 3/8] doc: kernel_abi.py: convert to sphinx.util.logging

[Mauro Carvalho Chehab]

Em Mon,  5 Feb 2024 18:51:28 +0100
Vegard Nossum <vegard.nossum@oracle.com> escreveu:

> As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
> to 2.4.4"), we can use Sphinx's built-in logging facilities.
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  Documentation/sphinx/kernel_abi.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/sphinx/kernel_abi.py b/Documentation/sphinx/kernel_abi.py
> index 288f26097569..9eb7282cc941 100644
> --- a/Documentation/sphinx/kernel_abi.py
> +++ b/Documentation/sphinx/kernel_abi.py
> @@ -37,16 +37,18 @@ import os
>  import subprocess
>  import sys
>  import re
> -import kernellog
>  
>  from docutils import nodes, statemachine
>  from docutils.statemachine import ViewList
>  from docutils.parsers.rst import directives, Directive
>  from docutils.utils.error_reporting import ErrorString
> +from sphinx.util import logging
>  from sphinx.util.docutils import switch_source_input
>  
>  __version__  = '1.0'
>  
> +logger = logging.getLogger(__name__)
> +
>  def setup(app):
>  
>      app.add_directive("kernel-abi", KernelCmd)
> @@ -129,7 +131,7 @@ class KernelCmd(Directive):
>              else:
>                  content.append(line, f, ln)
>  
> -        kernellog.info(self.state.document.settings.env.app, "%s: parsed %i lines" % (fname, n))
> +        logger.info("%s: parsed %i lines", fname, n)
>  
>          if content:
>              self.do_parse(content, node)



Thanks,
Mauro

Re: [PATCH 4/8] doc: kernel_feat.py: convert to sphinx.util.logging

[Mauro Carvalho Chehab]

Em Mon,  5 Feb 2024 18:51:29 +0100
Vegard Nossum <vegard.nossum@oracle.com> escreveu:

> As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
> to 2.4.4"), we can use Sphinx's built-in logging facilities.
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/sphinx/kernel_feat.py | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
> index 3493621d1a4e..f1c9e4a54964 100644
> --- a/Documentation/sphinx/kernel_feat.py
> +++ b/Documentation/sphinx/kernel_feat.py
> @@ -41,10 +41,13 @@ from docutils import nodes, statemachine
>  from docutils.statemachine import ViewList
>  from docutils.parsers.rst import directives, Directive
>  from docutils.utils.error_reporting import ErrorString
> +from sphinx.util import logging
>  from sphinx.util.docutils import switch_source_input
>  
>  __version__  = '1.0'
>  
> +logger = logging.getLogger(__name__)
> +
>  def setup(app):
>  
>      app.add_directive("kernel-feat", KernelFeat)
> @@ -67,12 +70,6 @@ class KernelFeat(Directive):
>          "debug"     : directives.flag
>      }
>  
> -    def warn(self, message, **replace):
> -        replace["fname"]   = self.state.document.current_source
> -        replace["line_no"] = replace.get("line_no", self.lineno)
> -        message = ("%(fname)s:%(line_no)s: [kernel-feat WARN] : " + message) % replace
> -        self.state.document.settings.env.app.warn(message, prefix="")
> -

That doesn't sound right.

If you remove the logic which gets the actual file name and line where
the error/warning have occurred, how are you handing now the special
output with such data produced by get_abi.pl to return the real file
name/line number where the error occurred?

Had you test changing an ABI file to cause a Sphinx warning and
ensured that the produced warning will report the actual location
of the warning, instead of shooting the messenger?

>      def run(self):
>          doc = self.state.document
>          if not doc.settings.file_insertion_enabled:

Thanks,
Mauro

Re: [PATCH 5/8] doc: kerneldoc.py: convert to sphinx.util.logging

[Mauro Carvalho Chehab]

Em Mon,  5 Feb 2024 18:51:30 +0100
Vegard Nossum <vegard.nossum@oracle.com> escreveu:

> As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
> to 2.4.4"), we can use Sphinx's built-in logging facilities.
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  Documentation/sphinx/kerneldoc.py | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> index 7acf09963daa..c74a3e417cea 100644
> --- a/Documentation/sphinx/kerneldoc.py
> +++ b/Documentation/sphinx/kerneldoc.py
> @@ -38,11 +38,13 @@ from docutils import nodes, statemachine
>  from docutils.statemachine import ViewList
>  from docutils.parsers.rst import directives, Directive
>  import sphinx
> +from sphinx.util import logging
>  from sphinx.util.docutils import switch_source_input
> -import kernellog
>  
>  __version__  = '1.0'
>  
> +logger = logging.getLogger(__name__)
> +
>  class KernelDocDirective(Directive):
>      """Extract kernel-doc comments from the specified file"""
>      required_argument = 1
> @@ -109,8 +111,7 @@ class KernelDocDirective(Directive):
>          cmd += [filename]
>  
>          try:
> -            kernellog.verbose(env.app,
> -                              'calling kernel-doc \'%s\'' % (" ".join(cmd)))
> +            logger.verbose('calling kernel-doc \'%s\'', " ".join(cmd))
>  
>              p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>              out, err = p.communicate()
> @@ -120,8 +121,7 @@ class KernelDocDirective(Directive):
>              if p.returncode != 0:
>                  sys.stderr.write(err)
>  
> -                kernellog.warn(env.app,
> -                               'kernel-doc \'%s\' failed with return code %d' % (" ".join(cmd), p.returncode))
> +                logger.warning('kernel-doc \'%s\' failed with return code %d', " ".join(cmd), p.returncode)
>                  return [nodes.error(None, nodes.paragraph(text = "kernel-doc missing"))]
>              elif env.config.kerneldoc_verbosity > 0:
>                  sys.stderr.write(err)
> @@ -148,8 +148,8 @@ class KernelDocDirective(Directive):
>              return node.children
>  
>          except Exception as e:  # pylint: disable=W0703
> -            kernellog.warn(env.app, 'kernel-doc \'%s\' processing failed with: %s' %
> -                           (" ".join(cmd), str(e)))
> +            logger.warning('kernel-doc \'%s\' processing failed with: %s',
> +                           " ".join(cmd), str(e))
>              return [nodes.error(None, nodes.paragraph(text = "kernel-doc missing"))]
>  
>      def do_parse(self, result, node):



Thanks,
Mauro

Re: [PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Mauro Carvalho Chehab]

Em Mon,  5 Feb 2024 18:51:31 +0100
Vegard Nossum <vegard.nossum@oracle.com> escreveu:

> As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
> to 2.4.4"), we can use Sphinx's built-in logging facilities.
> 
> Gotchas:
> - remove first argument 'app' from all calls
> - instead of (fmt % (args)), use (fmt, args)
> - instead of ("<fmt>: " + str) use ("<fmt: %s>", str)
> - realign wrapped lines
> 
> I changed the "Neither inkscape(1) nor convert(1) found." message from a
> .verbose() to a .warning(), since that actually affects the output in a
> big way.
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/sphinx/kfigure.py | 66 ++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/Documentation/sphinx/kfigure.py b/Documentation/sphinx/kfigure.py
> index 97166333b727..b58f6458af63 100644
> --- a/Documentation/sphinx/kfigure.py
> +++ b/Documentation/sphinx/kfigure.py
> @@ -58,13 +58,15 @@ from docutils.statemachine import ViewList
>  from docutils.parsers.rst import directives
>  from docutils.parsers.rst.directives import images
>  import sphinx
> +from sphinx.util import logging
>  from sphinx.util.nodes import clean_astext
> -import kernellog
>  
>  Figure = images.Figure
>  
>  __version__  = '1.0.0'
>  
> +logger = logging.getLogger(__name__)
> +
>  # simple helper
>  # -------------
>  
> @@ -170,7 +172,7 @@ def setupTools(app):
>      """
>      global dot_cmd, dot_Tpdf, convert_cmd, rsvg_convert_cmd   # pylint: disable=W0603
>      global inkscape_cmd, inkscape_ver_one  # pylint: disable=W0603
> -    kernellog.verbose(app, "kfigure: check installed tools ...")
> +    logger.verbose("kfigure: check installed tools ...")
>  
>      dot_cmd = which('dot')
>      convert_cmd = which('convert')
> @@ -178,7 +180,7 @@ def setupTools(app):
>      inkscape_cmd = which('inkscape')
>  
>      if dot_cmd:
> -        kernellog.verbose(app, "use dot(1) from: " + dot_cmd)
> +        logger.verbose("use dot(1) from: %s", dot_cmd)
>  
>          try:
>              dot_Thelp_list = subprocess.check_output([dot_cmd, '-Thelp'],
> @@ -190,10 +192,10 @@ def setupTools(app):
>          dot_Tpdf_ptn = b'pdf'
>          dot_Tpdf = re.search(dot_Tpdf_ptn, dot_Thelp_list)
>      else:
> -        kernellog.warn(app, "dot(1) not found, for better output quality install "
> +        logger.warning("dot(1) not found, for better output quality install "
>                         "graphviz from https://www.graphviz.org")
>      if inkscape_cmd:
> -        kernellog.verbose(app, "use inkscape(1) from: " + inkscape_cmd)
> +        logger.verbose("use inkscape(1) from: %s", inkscape_cmd)
>          inkscape_ver = subprocess.check_output([inkscape_cmd, '--version'],
>                                                 stderr=subprocess.DEVNULL)
>          ver_one_ptn = b'Inkscape 1'
> @@ -204,26 +206,26 @@ def setupTools(app):
>  
>      else:
>          if convert_cmd:
> -            kernellog.verbose(app, "use convert(1) from: " + convert_cmd)
> +            logger.verbose("use convert(1) from: %s", convert_cmd)
>          else:
> -            kernellog.verbose(app,
> +            logger.warning(
>                  "Neither inkscape(1) nor convert(1) found.\n"
>                  "For SVG to PDF conversion, "

Nitpick:

While you're here, please change the logic to not end likes with a "(",
e. g.:

            logger.warning("Neither inkscape(1) nor convert(1) found.\n"
                           "For SVG to PDF conversion, "
	    ...

Same to other long strings.

With that:

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>


>                  "install either Inkscape (https://inkscape.org/) (preferred) or\n"
>                  "ImageMagick (https://www.imagemagick.org)")
>  
>          if rsvg_convert_cmd:
> -            kernellog.verbose(app, "use rsvg-convert(1) from: " + rsvg_convert_cmd)
> -            kernellog.verbose(app, "use 'dot -Tsvg' and rsvg-convert(1) for DOT -> PDF conversion")
> +            logger.verbose("use rsvg-convert(1) from: %s", rsvg_convert_cmd)
> +            logger.verbose("use 'dot -Tsvg' and rsvg-convert(1) for DOT -> PDF conversion")
>              dot_Tpdf = False
>          else:
> -            kernellog.verbose(app,
> +            logger.verbose(
>                  "rsvg-convert(1) not found.\n"
>                  "  SVG rendering of convert(1) is done by ImageMagick-native renderer.")
>              if dot_Tpdf:
> -                kernellog.verbose(app, "use 'dot -Tpdf' for DOT -> PDF conversion")
> +                logger.verbose("use 'dot -Tpdf' for DOT -> PDF conversion")
>              else:
> -                kernellog.verbose(app, "use 'dot -Tsvg' and convert(1) for DOT -> PDF conversion")
> +                logger.verbose("use 'dot -Tsvg' and convert(1) for DOT -> PDF conversion")
>  
>  
>  # integrate conversion tools
> @@ -257,13 +259,12 @@ def convert_image(img_node, translator, src_fname=None):
>  
>      # in kernel builds, use 'make SPHINXOPTS=-v' to see verbose messages
>  
> -    kernellog.verbose(app, 'assert best format for: ' + img_node['uri'])
> +    logger.verbose('assert best format for: %s', img_node['uri'])
>  
>      if in_ext == '.dot':
>  
>          if not dot_cmd:
> -            kernellog.verbose(app,
> -                              "dot from graphviz not available / include DOT raw.")
> +            logger.verbose("dot from graphviz not available / include DOT raw.")
>              img_node.replace_self(file2literal(src_fname))
>  
>          elif translator.builder.format == 'latex':
> @@ -290,10 +291,9 @@ def convert_image(img_node, translator, src_fname=None):
>  
>          if translator.builder.format == 'latex':
>              if not inkscape_cmd and convert_cmd is None:
> -                kernellog.warn(app,
> -                                  "no SVG to PDF conversion available / include SVG raw."
> -                                  "\nIncluding large raw SVGs can cause xelatex error."
> -                                  "\nInstall Inkscape (preferred) or ImageMagick.")
> +                logger.warning("no SVG to PDF conversion available / include SVG raw.\n"
> +                               "Including large raw SVGs can cause xelatex error.\n"
> +                               "Install Inkscape (preferred) or ImageMagick.")
>                  img_node.replace_self(file2literal(src_fname))
>              else:
>                  dst_fname = path.join(translator.builder.outdir, fname + '.pdf')
> @@ -306,15 +306,14 @@ def convert_image(img_node, translator, src_fname=None):
>          _name = dst_fname[len(str(translator.builder.outdir)) + 1:]
>  
>          if isNewer(dst_fname, src_fname):
> -            kernellog.verbose(app,
> -                              "convert: {out}/%s already exists and is newer" % _name)
> +            logger.verbose("convert: {out}/%s already exists and is newer" % _name)
>  
>          else:
>              ok = False
>              mkdir(path.dirname(dst_fname))
>  
>              if in_ext == '.dot':
> -                kernellog.verbose(app, 'convert DOT to: {out}/' + _name)
> +                logger.verbose('convert DOT to: {out}/%s', _name)
>                  if translator.builder.format == 'latex' and not dot_Tpdf:
>                      svg_fname = path.join(translator.builder.outdir, fname + '.svg')
>                      ok1 = dot2format(app, src_fname, svg_fname)
> @@ -325,7 +324,7 @@ def convert_image(img_node, translator, src_fname=None):
>                      ok = dot2format(app, src_fname, dst_fname)
>  
>              elif in_ext == '.svg':
> -                kernellog.verbose(app, 'convert SVG to: {out}/' + _name)
> +                logger.verbose('convert SVG to: {out}/%s', _name)
>                  ok = svg2pdf(app, src_fname, dst_fname)
>  
>              if not ok:
> @@ -354,8 +353,7 @@ def dot2format(app, dot_fname, out_fname):
>      with open(out_fname, "w") as out:
>          exit_code = subprocess.call(cmd, stdout = out)
>          if exit_code != 0:
> -            kernellog.warn(app,
> -                          "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
> +            logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
>      return bool(exit_code == 0)
>  
>  def svg2pdf(app, svg_fname, pdf_fname):
> @@ -388,13 +386,13 @@ def svg2pdf(app, svg_fname, pdf_fname):
>          pass
>  
>      if exit_code != 0:
> -        kernellog.warn(app, "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
> +        logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
>          if warning_msg:
> -            kernellog.warn(app, "Warning msg from %s: %s"
> -                           % (cmd_name, str(warning_msg, 'utf-8')))
> +            logger.warning("Warning msg from %s: %s",
> +                           cmd_name, str(warning_msg, 'utf-8'))
>      elif warning_msg:
> -        kernellog.verbose(app, "Warning msg from %s (likely harmless):\n%s"
> -                          % (cmd_name, str(warning_msg, 'utf-8')))
> +        logger.warning("Warning msg from %s (likely harmless):\n%s",
> +                       cmd_name, str(warning_msg, 'utf-8'))
>  
>      return bool(exit_code == 0)
>  
> @@ -418,7 +416,7 @@ def svg2pdf_by_rsvg(app, svg_fname, pdf_fname):
>          # use stdout and stderr from parent
>          exit_code = subprocess.call(cmd)
>          if exit_code != 0:
> -            kernellog.warn(app, "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
> +            logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
>          ok = bool(exit_code == 0)
>  
>      return ok
> @@ -513,15 +511,15 @@ def visit_kernel_render(self, node):
>      app = self.builder.app
>      srclang = node.get('srclang')
>  
> -    kernellog.verbose(app, 'visit kernel-render node lang: "%s"' % (srclang))
> +    logger.verbose('visit kernel-render node lang: "%s"', srclang)
>  
>      tmp_ext = RENDER_MARKUP_EXT.get(srclang, None)
>      if tmp_ext is None:
> -        kernellog.warn(app, 'kernel-render: "%s" unknown / include raw.' % (srclang))
> +        logger.warning('kernel-render: "%s" unknown / include raw.', srclang)
>          return
>  
>      if not dot_cmd and tmp_ext == '.dot':
> -        kernellog.verbose(app, "dot from graphviz not available / include raw.")
> +        logger.verbose("dot from graphviz not available / include raw.")
>          return
>  
>      literal_block = node[0]



Thanks,
Mauro

Re: [PATCH 7/8] doc: remove kernellog.py

[Mauro Carvalho Chehab]

Em Mon,  5 Feb 2024 18:51:32 +0100
Vegard Nossum <vegard.nossum@oracle.com> escreveu:

> With the last kernellog users gone, we can remove this compatibility
> layer.
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  Documentation/sphinx/kernellog.py | 22 ----------------------
>  1 file changed, 22 deletions(-)
>  delete mode 100644 Documentation/sphinx/kernellog.py
> 
> diff --git a/Documentation/sphinx/kernellog.py b/Documentation/sphinx/kernellog.py
> deleted file mode 100644
> index 0bc00c138cad..000000000000
> --- a/Documentation/sphinx/kernellog.py
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -# SPDX-License-Identifier: GPL-2.0
> -#
> -# Sphinx has deprecated its older logging interface, but the replacement
> -# only goes back to 1.6.  So here's a wrapper layer to keep around for
> -# as long as we support 1.4.
> -#
> -# We don't support 1.4 anymore, but we'll keep the wrappers around until
> -# we change all the code to not use them anymore :)
> -#
> -import sphinx
> -from sphinx.util import logging
> -
> -logger = logging.getLogger('kerneldoc')
> -
> -def warn(app, message):
> -    logger.warning(message)
> -
> -def verbose(app, message):
> -    logger.verbose(message)
> -
> -def info(app, message):
> -    logger.info(message)



Thanks,
Mauro

Re: [PATCH 8/8] doc: kernel_{abi,feat}.py: warn about missing directory

[Mauro Carvalho Chehab]

Em Mon,  5 Feb 2024 18:51:33 +0100
Vegard Nossum <vegard.nossum@oracle.com> escreveu:

> The underlying perl script get_feat.pl does not complain when you pass
> nonexistent directories, and just produces empty/useless output:
> 
>     $ scripts/get_feat.pl rest --enable-fname --dir asdfasdf --arch arm64
>     ====================================
>     Feature status on arm64 architecture
>     ====================================
> 
>     =========  =======  =======  ======  ===========
>     Subsystem  Feature  Kconfig  Status  Description
>     =========  =======  =======  ======  ===========
>     =========  =======  =======  ======  ===========
> 
> Let's add an early sanity check and a warning if the check fails.
> 
> get_abi.pl doesn't have exactly the same issue as it actually produces
> an error, but we can add the same check for consistency.

Fixing it is a good thing, but IMO the change should happen instead
at get_abi.pl and get_feat.pl level, as it is a way more likely
that someone will misuse it when running the tools via command
line than when modifying the places where they're called.

> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/sphinx/kernel_abi.py  | 7 ++++++-
>  Documentation/sphinx/kernel_feat.py | 8 ++++++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/sphinx/kernel_abi.py b/Documentation/sphinx/kernel_abi.py
> index 9eb7282cc941..52af2750e634 100644
> --- a/Documentation/sphinx/kernel_abi.py
> +++ b/Documentation/sphinx/kernel_abi.py
> @@ -79,11 +79,16 @@ class KernelCmd(Directive):
>  
>          srctree = os.path.abspath(os.environ["srctree"])
>  
> +        dir_path = os.path.join(srctree, 'Documentation', self.arguments[0])
> +        if not os.path.exists(dir_path):
> +            logger.warning("path does not exist: '%s'", dir_path,
> +                           location=(self.state.document.current_source, self.lineno))
> +
>          args = [
>              os.path.join(srctree, 'scripts/get_abi.pl'),
>              'rest',
>              '--enable-lineno',
> -            '--dir', os.path.join(srctree, 'Documentation', self.arguments[0]),
> +            '--dir', dir_path,
>          ]
>  
>          if 'rst' in self.options:
> diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
> index f1c9e4a54964..e0bc6e03579c 100644
> --- a/Documentation/sphinx/kernel_feat.py
> +++ b/Documentation/sphinx/kernel_feat.py
> @@ -79,12 +79,16 @@ class KernelFeat(Directive):
>  
>          srctree = os.path.abspath(os.environ["srctree"])
>  
> +        dir_path = os.path.join(srctree, 'Documentation', self.arguments[0])
> +        if not os.path.exists(dir_path):
> +            logger.warning("path does not exist: '%s'", dir_path,
> +                           location=(self.state.document.current_source, self.lineno))
> +
>          args = [
>              os.path.join(srctree, 'scripts/get_feat.pl'),
>              'rest',
>              '--enable-fname',
> -            '--dir',
> -            os.path.join(srctree, 'Documentation', self.arguments[0]),
> +            '--dir', dir_path,
>          ]
>  
>          if len(self.arguments) > 1:



Thanks,
Mauro

Re: [PATCH 1/8] docs: kernel_feat.py: fix build error for missing files

[Salvatore Bonaccorso]

Hi Vegard,

On Mon, Feb 05, 2024 at 06:51:26PM +0100, Vegard Nossum wrote:
> If the directory passed to the '.. kernel-feat::' directive does not
> exist or the get_feat.pl script does not find any files to extract
> features from, Sphinx will report the following error:
> 
>     Sphinx parallel build error:
>     UnboundLocalError: local variable 'fname' referenced before assignment
>     make[2]: *** [Documentation/Makefile:102: htmldocs] Error 2
> 
> This is due to how I changed the script in c48a7c44a1d0 ("docs:
> kernel_feat.py: fix potential command injection"). Before that, the
> filename passed along to self.nestedParse() in this case was weirdly
> just the whole get_feat.pl invocation.
> 
> We can fix it by doing what kernel_abi.py does -- just pass
> self.arguments[0] as 'fname'.
> 
> Fixes: c48a7c44a1d0 ("docs: kernel_feat.py: fix potential command injection")
> Cc: Justin Forbes <jforbes@fedoraproject.org>
> Cc: Salvatore Bonaccorso <carnil@debian.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/sphinx/kernel_feat.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
> index b9df61eb4501..03ace5f01b5c 100644
> --- a/Documentation/sphinx/kernel_feat.py
> +++ b/Documentation/sphinx/kernel_feat.py
> @@ -109,7 +109,7 @@ class KernelFeat(Directive):
>              else:
>                  out_lines += line + "\n"
>  
> -        nodeList = self.nestedParse(out_lines, fname)
> +        nodeList = self.nestedParse(out_lines, self.arguments[0])
>          return nodeList
>  
>      def nestedParse(self, lines, fname):
> -- 
> 2.34.1

Thanks for the fix. Tested doc build on top of v6.6.16 and addresses
the issue.

Tested-by: Salvatore Bonaccorso <carnil@debian.org>

Regards,
Salvatore

Re: [PATCH 2/8] docs: kernel_{abi,feat}.py: use doc.current_source

[Jani Nikula]

On Mon, 05 Feb 2024, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> It probably doesn't matter a whole lot what we actually pass here,
> but the .rst being processed seems most appropriate to me.

But it's not the actual .rst file being parsed here. It's something
originating from some oher file and processed in between. The kernel-doc
extension takes care to map the parsing errors in source code comments
to the right source file and line, which is where the problem is, not in
the .rst file.

The line numbers in the error messages will be adjusted according to the
ViewList. So I don't think you'll get messages that actually point at
line where the directive is either.

Please experiment with some errors injected to see what the output will
be.


BR,
Jani.

>
> This presumably gets used by Shpinx to record/report where each line
> of .rst source originates.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/sphinx/kernel_abi.py  | 2 +-
>  Documentation/sphinx/kernel_feat.py | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/sphinx/kernel_abi.py b/Documentation/sphinx/kernel_abi.py
> index 5911bd0d7965..288f26097569 100644
> --- a/Documentation/sphinx/kernel_abi.py
> +++ b/Documentation/sphinx/kernel_abi.py
> @@ -88,7 +88,7 @@ class KernelCmd(Directive):
>              args.append('--rst-source')
>  
>          lines = subprocess.check_output(args, cwd=os.path.dirname(doc.current_source)).decode('utf-8')
> -        nodeList = self.nestedParse(lines, self.arguments[0])
> +        nodeList = self.nestedParse(lines, doc.current_source)
>          return nodeList
>  
>      def nestedParse(self, lines, fname):
> diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
> index 03ace5f01b5c..3493621d1a4e 100644
> --- a/Documentation/sphinx/kernel_feat.py
> +++ b/Documentation/sphinx/kernel_feat.py
> @@ -109,7 +109,7 @@ class KernelFeat(Directive):
>              else:
>                  out_lines += line + "\n"
>  
> -        nodeList = self.nestedParse(out_lines, self.arguments[0])
> +        nodeList = self.nestedParse(out_lines, doc.current_source)
>          return nodeList
>  
>      def nestedParse(self, lines, fname):

-- 
Jani Nikula, Intel

Re: [PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Jani Nikula]

On Mon, 05 Feb 2024, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
> to 2.4.4"), we can use Sphinx's built-in logging facilities.
>
> Gotchas:
> - remove first argument 'app' from all calls
> - instead of (fmt % (args)), use (fmt, args)
> - instead of ("<fmt>: " + str) use ("<fmt: %s>", str)

If you're doing this, why not go directly to f-strings? IMO the above
are inferior to it.

> - realign wrapped lines
>
> I changed the "Neither inkscape(1) nor convert(1) found." message from a
> .verbose() to a .warning(), since that actually affects the output in a
> big way.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/sphinx/kfigure.py | 66 ++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/sphinx/kfigure.py b/Documentation/sphinx/kfigure.py
> index 97166333b727..b58f6458af63 100644
> --- a/Documentation/sphinx/kfigure.py
> +++ b/Documentation/sphinx/kfigure.py
> @@ -58,13 +58,15 @@ from docutils.statemachine import ViewList
>  from docutils.parsers.rst import directives
>  from docutils.parsers.rst.directives import images
>  import sphinx
> +from sphinx.util import logging
>  from sphinx.util.nodes import clean_astext
> -import kernellog
>  
>  Figure = images.Figure
>  
>  __version__  = '1.0.0'
>  
> +logger = logging.getLogger(__name__)
> +
>  # simple helper
>  # -------------
>  
> @@ -170,7 +172,7 @@ def setupTools(app):
>      """
>      global dot_cmd, dot_Tpdf, convert_cmd, rsvg_convert_cmd   # pylint: disable=W0603
>      global inkscape_cmd, inkscape_ver_one  # pylint: disable=W0603
> -    kernellog.verbose(app, "kfigure: check installed tools ...")
> +    logger.verbose("kfigure: check installed tools ...")
>  
>      dot_cmd = which('dot')
>      convert_cmd = which('convert')
> @@ -178,7 +180,7 @@ def setupTools(app):
>      inkscape_cmd = which('inkscape')
>  
>      if dot_cmd:
> -        kernellog.verbose(app, "use dot(1) from: " + dot_cmd)
> +        logger.verbose("use dot(1) from: %s", dot_cmd)

For example:

logger.verbose(f"use dot(1) from: {dot_cmd}")

>  
>          try:
>              dot_Thelp_list = subprocess.check_output([dot_cmd, '-Thelp'],
> @@ -190,10 +192,10 @@ def setupTools(app):
>          dot_Tpdf_ptn = b'pdf'
>          dot_Tpdf = re.search(dot_Tpdf_ptn, dot_Thelp_list)
>      else:
> -        kernellog.warn(app, "dot(1) not found, for better output quality install "
> +        logger.warning("dot(1) not found, for better output quality install "
>                         "graphviz from https://www.graphviz.org")
>      if inkscape_cmd:
> -        kernellog.verbose(app, "use inkscape(1) from: " + inkscape_cmd)
> +        logger.verbose("use inkscape(1) from: %s", inkscape_cmd)
>          inkscape_ver = subprocess.check_output([inkscape_cmd, '--version'],
>                                                 stderr=subprocess.DEVNULL)
>          ver_one_ptn = b'Inkscape 1'
> @@ -204,26 +206,26 @@ def setupTools(app):
>  
>      else:
>          if convert_cmd:
> -            kernellog.verbose(app, "use convert(1) from: " + convert_cmd)
> +            logger.verbose("use convert(1) from: %s", convert_cmd)
>          else:
> -            kernellog.verbose(app,
> +            logger.warning(
>                  "Neither inkscape(1) nor convert(1) found.\n"
>                  "For SVG to PDF conversion, "
>                  "install either Inkscape (https://inkscape.org/) (preferred) or\n"
>                  "ImageMagick (https://www.imagemagick.org)")

These could be converted to use """:

"""Neither inkscape(1) nor convert(1) found.
For SVG to PDF conversion, 
install either Inkscape (https://inkscape.org/) (preferred) or
ImageMagick (https://www.imagemagick.org)"
"""

>  
>          if rsvg_convert_cmd:
> -            kernellog.verbose(app, "use rsvg-convert(1) from: " + rsvg_convert_cmd)
> -            kernellog.verbose(app, "use 'dot -Tsvg' and rsvg-convert(1) for DOT -> PDF conversion")
> +            logger.verbose("use rsvg-convert(1) from: %s", rsvg_convert_cmd)
> +            logger.verbose("use 'dot -Tsvg' and rsvg-convert(1) for DOT -> PDF conversion")
>              dot_Tpdf = False
>          else:
> -            kernellog.verbose(app,
> +            logger.verbose(
>                  "rsvg-convert(1) not found.\n"
>                  "  SVG rendering of convert(1) is done by ImageMagick-native renderer.")
>              if dot_Tpdf:
> -                kernellog.verbose(app, "use 'dot -Tpdf' for DOT -> PDF conversion")
> +                logger.verbose("use 'dot -Tpdf' for DOT -> PDF conversion")
>              else:
> -                kernellog.verbose(app, "use 'dot -Tsvg' and convert(1) for DOT -> PDF conversion")
> +                logger.verbose("use 'dot -Tsvg' and convert(1) for DOT -> PDF conversion")
>  
>  
>  # integrate conversion tools
> @@ -257,13 +259,12 @@ def convert_image(img_node, translator, src_fname=None):
>  
>      # in kernel builds, use 'make SPHINXOPTS=-v' to see verbose messages
>  
> -    kernellog.verbose(app, 'assert best format for: ' + img_node['uri'])
> +    logger.verbose('assert best format for: %s', img_node['uri'])
>  
>      if in_ext == '.dot':
>  
>          if not dot_cmd:
> -            kernellog.verbose(app,
> -                              "dot from graphviz not available / include DOT raw.")
> +            logger.verbose("dot from graphviz not available / include DOT raw.")
>              img_node.replace_self(file2literal(src_fname))
>  
>          elif translator.builder.format == 'latex':
> @@ -290,10 +291,9 @@ def convert_image(img_node, translator, src_fname=None):
>  
>          if translator.builder.format == 'latex':
>              if not inkscape_cmd and convert_cmd is None:
> -                kernellog.warn(app,
> -                                  "no SVG to PDF conversion available / include SVG raw."
> -                                  "\nIncluding large raw SVGs can cause xelatex error."
> -                                  "\nInstall Inkscape (preferred) or ImageMagick.")
> +                logger.warning("no SVG to PDF conversion available / include SVG raw.\n"
> +                               "Including large raw SVGs can cause xelatex error.\n"
> +                               "Install Inkscape (preferred) or ImageMagick.")
>                  img_node.replace_self(file2literal(src_fname))
>              else:
>                  dst_fname = path.join(translator.builder.outdir, fname + '.pdf')
> @@ -306,15 +306,14 @@ def convert_image(img_node, translator, src_fname=None):
>          _name = dst_fname[len(str(translator.builder.outdir)) + 1:]
>  
>          if isNewer(dst_fname, src_fname):
> -            kernellog.verbose(app,
> -                              "convert: {out}/%s already exists and is newer" % _name)
> +            logger.verbose("convert: {out}/%s already exists and is newer" % _name)
>  
>          else:
>              ok = False
>              mkdir(path.dirname(dst_fname))
>  
>              if in_ext == '.dot':
> -                kernellog.verbose(app, 'convert DOT to: {out}/' + _name)
> +                logger.verbose('convert DOT to: {out}/%s', _name)
>                  if translator.builder.format == 'latex' and not dot_Tpdf:
>                      svg_fname = path.join(translator.builder.outdir, fname + '.svg')
>                      ok1 = dot2format(app, src_fname, svg_fname)
> @@ -325,7 +324,7 @@ def convert_image(img_node, translator, src_fname=None):
>                      ok = dot2format(app, src_fname, dst_fname)
>  
>              elif in_ext == '.svg':
> -                kernellog.verbose(app, 'convert SVG to: {out}/' + _name)
> +                logger.verbose('convert SVG to: {out}/%s', _name)
>                  ok = svg2pdf(app, src_fname, dst_fname)
>  
>              if not ok:
> @@ -354,8 +353,7 @@ def dot2format(app, dot_fname, out_fname):
>      with open(out_fname, "w") as out:
>          exit_code = subprocess.call(cmd, stdout = out)
>          if exit_code != 0:
> -            kernellog.warn(app,
> -                          "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
> +            logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
>      return bool(exit_code == 0)
>  
>  def svg2pdf(app, svg_fname, pdf_fname):
> @@ -388,13 +386,13 @@ def svg2pdf(app, svg_fname, pdf_fname):
>          pass
>  
>      if exit_code != 0:
> -        kernellog.warn(app, "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
> +        logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
>          if warning_msg:
> -            kernellog.warn(app, "Warning msg from %s: %s"
> -                           % (cmd_name, str(warning_msg, 'utf-8')))
> +            logger.warning("Warning msg from %s: %s",
> +                           cmd_name, str(warning_msg, 'utf-8'))
>      elif warning_msg:
> -        kernellog.verbose(app, "Warning msg from %s (likely harmless):\n%s"
> -                          % (cmd_name, str(warning_msg, 'utf-8')))
> +        logger.warning("Warning msg from %s (likely harmless):\n%s",
> +                       cmd_name, str(warning_msg, 'utf-8'))
>  
>      return bool(exit_code == 0)
>  
> @@ -418,7 +416,7 @@ def svg2pdf_by_rsvg(app, svg_fname, pdf_fname):
>          # use stdout and stderr from parent
>          exit_code = subprocess.call(cmd)
>          if exit_code != 0:
> -            kernellog.warn(app, "Error #%d when calling: %s" % (exit_code, " ".join(cmd)))
> +            logger.warning("Error #%d when calling: %s", exit_code, " ".join(cmd))
>          ok = bool(exit_code == 0)
>  
>      return ok
> @@ -513,15 +511,15 @@ def visit_kernel_render(self, node):
>      app = self.builder.app
>      srclang = node.get('srclang')
>  
> -    kernellog.verbose(app, 'visit kernel-render node lang: "%s"' % (srclang))
> +    logger.verbose('visit kernel-render node lang: "%s"', srclang)
>  
>      tmp_ext = RENDER_MARKUP_EXT.get(srclang, None)
>      if tmp_ext is None:
> -        kernellog.warn(app, 'kernel-render: "%s" unknown / include raw.' % (srclang))
> +        logger.warning('kernel-render: "%s" unknown / include raw.', srclang)
>          return
>  
>      if not dot_cmd and tmp_ext == '.dot':
> -        kernellog.verbose(app, "dot from graphviz not available / include raw.")
> +        logger.verbose("dot from graphviz not available / include raw.")
>          return
>  
>      literal_block = node[0]

-- 
Jani Nikula, Intel

Re: [PATCH 4/8] doc: kernel_feat.py: convert to sphinx.util.logging

[Vegard Nossum]

On 06/02/2024 05:42, Mauro Carvalho Chehab wrote:
> Em Mon,  5 Feb 2024 18:51:29 +0100
> Vegard Nossum <vegard.nossum@oracle.com> escreveu:
>> @@ -67,12 +70,6 @@ class KernelFeat(Directive):
>>           "debug"     : directives.flag
>>       }
>>   
>> -    def warn(self, message, **replace):
>> -        replace["fname"]   = self.state.document.current_source
>> -        replace["line_no"] = replace.get("line_no", self.lineno)
>> -        message = ("%(fname)s:%(line_no)s: [kernel-feat WARN] : " + message) % replace
>> -        self.state.document.settings.env.app.warn(message, prefix="")
>> -
> 
> That doesn't sound right.
> 
> If you remove the logic which gets the actual file name and line where
> the error/warning have occurred, how are you handing now the special
> output with such data produced by get_abi.pl to return the real file
> name/line number where the error occurred?
> 
> Had you test changing an ABI file to cause a Sphinx warning and
> ensured that the produced warning will report the actual location
> of the warning, instead of shooting the messenger?

Sorry, I should have described this change better.

I don't think this warn() method is called at all from anywhere --
removing it here was meant as pure cleanup.

Maybe I don't understand the mechanism, though. Is this called
indirectly somewhere through Sphinx? I see there's a warning() method in
the Directive class, but this is self.warn() we're talking about.

(BTW, this is in kernel_feat.py -- not kernel_abi.py.)

If I add some bad syntax (like :doc:`) to one of the
Documentation/features/ descriptions, I get a warning like this:

/home/vegard/linux/Documentation/arch/m68k/features.rst:23: WARNING: 
Inline interpreted text or phrase reference start-string without end-string.

I've verified that this does not change with this patch; kernel_feat.py
has always reported source warnings in this way; self.warn() has been
unused since the extension was added, as far as I can tell.

(kernel_abi.py is a different story, it attempts to report the correct
source lines, but I'm not convinced that it works properly either.
Tackling both scripts to generate correct warnings is next on my TODO,
but out of scope for this series.)


Vegard

Re: [PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Vegard Nossum]

On 06/02/2024 04:04, Akira Yokosawa wrote:
> On Mon,  5 Feb 2024 18:51:31 +0100, Vegard Nossum wrote:
>> As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
>> to 2.4.4"), we can use Sphinx's built-in logging facilities.
>>
>> Gotchas:
>> - remove first argument 'app' from all calls
>> - instead of (fmt % (args)), use (fmt, args)
>> - instead of ("<fmt>: " + str) use ("<fmt: %s>", str)
>> - realign wrapped lines
>>
>> I changed the "Neither inkscape(1) nor convert(1) found." message from a
>> .verbose() to a .warning(), since that actually affects the output in a
>> big way.
> 
> No, please don't!
> 
> you are partially reverting commit d987d5ae51ec ("docs: kfigure.py:
> Don't warn of missing PDF converter in 'make htmldocs'").
> 
> See its changelog for why it should be kept verbase.

I see, thanks for pointing it out, I'll fix that in v2.


Vegard

Re: [PATCH 2/8] docs: kernel_{abi,feat}.py: use doc.current_source

[Vegard Nossum]

On 06/02/2024 09:49, Jani Nikula wrote:
> On Mon, 05 Feb 2024, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>> It probably doesn't matter a whole lot what we actually pass here,
>> but the .rst being processed seems most appropriate to me.
> 
> But it's not the actual .rst file being parsed here. It's something
> originating from some oher file and processed in between. The kernel-doc
> extension takes care to map the parsing errors in source code comments
> to the right source file and line, which is where the problem is, not in
> the .rst file.
> 
> The line numbers in the error messages will be adjusted according to the
> ViewList. So I don't think you'll get messages that actually point at
> line where the directive is either.
> 
> Please experiment with some errors injected to see what the output will
> be.

Thanks for the comment, also see my related response to Mauro here:

<https://lore.kernel.org/all/3fce153d-37c5-4c4d-a4b3-44e376daa095@oracle.com/>

TL;DR: there are several different issues in both kernel_abi.py and
kernel_feat.py related to error reporting, and it's on my TODO.

High-level explanation:

Both scripts register an rST directive which call out to a Perl script
(scripts/get_{abi,feat}.py, respectively). The Perl script generates a
mixture of rST syntax, some of which it generates itself on the fly and
some of which comes from auxiliary files (the Documentation/ABI/** and
Documentation/features/** files, respectively).

Now, the two scripts have diverged a little bit in their implementation
of exactly how they do this.

kernel_abi.py (get_abi.pl) is the one I would consider "more correct",
since it actually attempts to attribute the source lines correctly, by
taking the "top-level" filename as the 'fname' parameter to
nestedParse() and updating the current file/line number whenever it
encounters '.. LINENO:' from the Perl script's output.

What you say is true -- when there is a warning at the "top level"
(before any '.. LINENO:' directive has been encountered), kernel_abi.py
does not report the actual .rst file being parsed, it reports the
location of the '.. kernel-abi:' directive instead. This is what I
changed in this patch (2/8) -- I changed it from being
self.arguments[0], which is the argument to the .. kernel-abi:
directive, in most cases something like ABI/stable or ABI/obsolete,
which is not even an .rst file, to being the .rst file that contains the
directive. My rationale is that this is more helpful than just being
pointed to something that isn't even an .rst file.

Now, about kernel_feat.py (get_feat.pl), it has similar logic, but
worse, in a way: get_feat.pl outputs all its '.. FILE:' lines at the top
of its output, followed by the rest of the generated .rst content. So
there isn't really much point in trying to attribute these lines to the
"correct" file -- yes, we should probably fix get_feat.pl here. Anyway,
the script doesn't parse the '.. FILE:' directives in the same way, it
just attributes ALL the generated lines to self.arguments[0] (or
doc.current_source after this patch). The only thing it actually uses
those directives for is to tell Sphinx about dependencies so the output
is rebuilt if one of the sources change.

The bottom line is that this reporting has NEVER fully worked as intended.

I was attempting to push things in the right direction with this patch
series, but I didn't want to do too much at once since that makes it
even harder for the patches to get past the Great (Review) Filter.


Vegard

Re: [PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Vegard Nossum]

On 06/02/2024 09:57, Jani Nikula wrote:
> On Mon, 05 Feb 2024, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>> As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
>> to 2.4.4"), we can use Sphinx's built-in logging facilities.
>>
>> Gotchas:
>> - remove first argument 'app' from all calls
>> - instead of (fmt % (args)), use (fmt, args)
>> - instead of ("<fmt>: " + str) use ("<fmt: %s>", str)
> 
> If you're doing this, why not go directly to f-strings? IMO the above
> are inferior to it.

I agree, and I also vastly prefer f-strings everywhere(*) personally.

(*): However, for logging and logging-related APIs I had the impression
that it was still canonical/recommended not to use f-strings since they
are still going to get interpolated by the logging API -- which means if
you use an f-string and the interpolated f-string results in a %
anywhere your logging call will fail since it won't find the
corresponding parameter in the argument list -- similar to using
printf(buf); instead of printf("%s", buf); in C.

In other words, the change in this patch is not (purely) a stylistic
one, but also a correctness thing.


Vegard

Re: [PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Vegard Nossum]

On 06/02/2024 09:57, Jani Nikula wrote:
> On Mon, 05 Feb 2024, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>> -            kernellog.verbose(app,
>> +            logger.warning(
>>                   "Neither inkscape(1) nor convert(1) found.\n"
>>                   "For SVG to PDF conversion, "
>>                   "install either Inkscape (https://inkscape.org/) (preferred) or\n"
>>                   "ImageMagick (https://www.imagemagick.org)")
> 
> These could be converted to use """:
> 
> """Neither inkscape(1) nor convert(1) found.
> For SVG to PDF conversion,
> install either Inkscape (https://inkscape.org/) (preferred) or
> ImageMagick (https://www.imagemagick.org)"
> """

That would mean the lines could not be indented, causing a (to me)
somewhat strange-looking block:

             logger.verbose("""Neither inkscape(1) nor convert(1) found.
For SVG to PDF conversion, install either Inkscape 
(https://inkscape.org/) (preferred) or
ImageMagick (https://www.imagemagick.org)""")

Is that preferred?


Vegard

Re: [PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Mauro Carvalho Chehab]

Em Tue, 06 Feb 2024 10:57:36 +0200
Jani Nikula <jani.nikula@intel.com> escreveu:

> On Mon, 05 Feb 2024, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> > As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
> > to 2.4.4"), we can use Sphinx's built-in logging facilities.
> >
> > Gotchas:
> > - remove first argument 'app' from all calls
> > - instead of (fmt % (args)), use (fmt, args)
> > - instead of ("<fmt>: " + str) use ("<fmt: %s>", str)  
> 
> If you're doing this, why not go directly to f-strings? IMO the above
> are inferior to it.

Hmm... f-strings require at least python 3.6. Not sure what's the current
requirement.

On a quick check wit vermin:

<snip>
$ vermin -v $(git ls-files "*.py"|grep -v tools)

!2, 3.3      Documentation/conf.py
2.6, 3.0     Documentation/networking/device_drivers/atm/cxacru-cf.py
!2, 3.6      Documentation/sphinx/automarkup.py
2.4, 3.0     Documentation/sphinx/cdomain.py
2.7, 3.1     Documentation/sphinx/kernel_abi.py
2.7, 3.1     Documentation/sphinx/kernel_feat.py
2.3, 3.0     Documentation/sphinx/kernel_include.py
2.5, 3.0     Documentation/sphinx/kerneldoc.py
~2, ~3       Documentation/sphinx/kernellog.py
!2, 3.3      Documentation/sphinx/kfigure.py
2.5, 3.0     Documentation/sphinx/load_config.py
2.3, 3.0     Documentation/sphinx/maintainers_include.py
2.3, 3.0     Documentation/sphinx/rstFlatTable.py
!2, 3.0      Documentation/sphinx/translations.py
2.0, !3      Documentation/trace/postprocess/decode_msr.py
File with incompatible versions: Documentation/trace/postprocess/decode_msr.py
2.3, 3.0     Documentation/userspace-api/media/conf_nitpick.py
!2, 3.5      drivers/gpu/drm/ci/check-patch.py
!2, 3.6      drivers/gpu/drm/ci/xfails/update-xfails.py
!2, 3.7      scripts/bpf_doc.py
!2, 3.3      scripts/checkkconfigsymbols.py
~2, ~3       scripts/gdb/linux/__init__.py
2.2, 3.0     scripts/gdb/linux/clk.py
2.5, 3.0     scripts/gdb/linux/config.py
2.7, 3.0     scripts/gdb/linux/cpus.py
2.7, 3.0     scripts/gdb/linux/device.py
2.6, 3.0     scripts/gdb/linux/dmesg.py
2.7, 3.0     scripts/gdb/linux/genpd.py
2.7, 3.0     scripts/gdb/linux/interrupts.py
2.7, 3.0     scripts/gdb/linux/lists.py
2.3, 3.0     scripts/gdb/linux/mm.py
2.6, 3.0     scripts/gdb/linux/modules.py
2.3, 3.0     scripts/gdb/linux/page_owner.py
!2, 3.6      scripts/gdb/linux/pgtable.py
2.7, 3.0     scripts/gdb/linux/proc.py
2.7, 3.0     scripts/gdb/linux/radixtree.py
2.7, 3.0     scripts/gdb/linux/rbtree.py
2.7, 3.0     scripts/gdb/linux/slab.py
~2, ~3       scripts/gdb/linux/stackdepot.py
2.7, 3.0     scripts/gdb/linux/symbols.py
2.7, 3.0     scripts/gdb/linux/tasks.py
2.7, 3.0     scripts/gdb/linux/timerlist.py
2.7, 3.0     scripts/gdb/linux/utils.py
2.2, 3.0     scripts/gdb/linux/vfs.py
2.2, 3.0     scripts/gdb/linux/vmalloc.py
2.3, 3.0     scripts/gdb/vmlinux-gdb.py
!2, 3.6      scripts/generate_rust_analyzer.py
~2, ~3       scripts/kconfig/tests/auto_submenu/__init__.py
~2, ~3       scripts/kconfig/tests/choice/__init__.py
~2, ~3       scripts/kconfig/tests/choice_value_with_m_dep/__init__.py
!2, 3.2      scripts/kconfig/tests/conftest.py
~2, ~3       scripts/kconfig/tests/err_recursive_dep/__init__.py
~2, ~3       scripts/kconfig/tests/err_recursive_inc/__init__.py
~2, ~3       scripts/kconfig/tests/inter_choice/__init__.py
~2, ~3       scripts/kconfig/tests/new_choice_with_dep/__init__.py
~2, ~3       scripts/kconfig/tests/no_write_if_dep_unmet/__init__.py
~2, ~3       scripts/kconfig/tests/preprocess/builtin_func/__init__.py
~2, ~3       scripts/kconfig/tests/preprocess/circular_expansion/__init__.py
~2, ~3       scripts/kconfig/tests/preprocess/escape/__init__.py
~2, ~3       scripts/kconfig/tests/preprocess/variable/__init__.py
!2, 3.9      scripts/rust_is_available_test.py
!2, 3.2      scripts/spdxcheck.py
2.3, 3.0     scripts/tracing/draw_functrace.py
</snip>

It sounds that automarkup is already requiring 3.6, so it should be ok
to use it here too (also because it uses f-strings):

	$ vermin -vv Documentation/sphinx/automarkup.py 
	Detecting python files..
	Analyzing using 8 processes..
	!2, 3.6      /new_devel/v4l/media_tree/Documentation/sphinx/automarkup.py
	  'itertools' module requires 2.3, 3.0
	  'sorted' member requires 2.4, 3.0
	  'sorted(key)' requires 2.4, 3.0
	  `with` requires 2.5, 3.0
	  f-strings require !2, 3.6
	...

There are still:

	scripts/bpf_doc.py requires 3.7
	rust_is_available_test.py requires 3.9.

But those don't seem to be part of the build.


Anyway, I would expect that the minimal python version to be listed at:

	Documentation/process/changes.rst

Apparently, it isn't. IMO, we need to document there that python
3.6 is the minimal version required to build the Kernel - or
at least the documentation.


Thanks,
Mauro

Re: [PATCH 6/8] doc: kfigure.py: convert to sphinx.util.logging

[Jani Nikula]

On Tue, 06 Feb 2024, Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> Em Tue, 06 Feb 2024 10:57:36 +0200
> Jani Nikula <jani.nikula@intel.com> escreveu:
>
>> On Mon, 05 Feb 2024, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>> > As of commit 3e893e16af55 ("docs: Raise the minimum Sphinx requirement
>> > to 2.4.4"), we can use Sphinx's built-in logging facilities.
>> >
>> > Gotchas:
>> > - remove first argument 'app' from all calls
>> > - instead of (fmt % (args)), use (fmt, args)
>> > - instead of ("<fmt>: " + str) use ("<fmt: %s>", str)  
>> 
>> If you're doing this, why not go directly to f-strings? IMO the above
>> are inferior to it.
>
> Hmm... f-strings require at least python 3.6. Not sure what's the current
> requirement.

[snip]

> Anyway, I would expect that the minimal python version to be listed at:
>
> 	Documentation/process/changes.rst
>
> Apparently, it isn't. IMO, we need to document there that python
> 3.6 is the minimal version required to build the Kernel - or
> at least the documentation.

Yeah, need to document the minimum version, but I'd rather bump it to a
supported version like 3.8 rather than something that reached end of
life two years ago [1].


BR,
Jani.


[1] https://devguide.python.org/versions/


-- 
Jani Nikula, Intel

Re: [PATCH 1/8] docs: kernel_feat.py: fix build error for missing files

[Jonathan Corbet]

Vegard Nossum <vegard.nossum@oracle.com> writes:

> If the directory passed to the '.. kernel-feat::' directive does not
> exist or the get_feat.pl script does not find any files to extract
> features from, Sphinx will report the following error:
>
>     Sphinx parallel build error:
>     UnboundLocalError: local variable 'fname' referenced before assignment
>     make[2]: *** [Documentation/Makefile:102: htmldocs] Error 2
>
> This is due to how I changed the script in c48a7c44a1d0 ("docs:
> kernel_feat.py: fix potential command injection"). Before that, the
> filename passed along to self.nestedParse() in this case was weirdly
> just the whole get_feat.pl invocation.
>
> We can fix it by doing what kernel_abi.py does -- just pass
> self.arguments[0] as 'fname'.
>
> Fixes: c48a7c44a1d0 ("docs: kernel_feat.py: fix potential command injection")
> Cc: Justin Forbes <jforbes@fedoraproject.org>
> Cc: Salvatore Bonaccorso <carnil@debian.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/sphinx/kernel_feat.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
> index b9df61eb4501..03ace5f01b5c 100644
> --- a/Documentation/sphinx/kernel_feat.py
> +++ b/Documentation/sphinx/kernel_feat.py
> @@ -109,7 +109,7 @@ class KernelFeat(Directive):
>              else:
>                  out_lines += line + "\n"
>  
> -        nodeList = self.nestedParse(out_lines, fname)
> +        nodeList = self.nestedParse(out_lines, self.arguments[0])
>          return nodeList

So I can certainly track this through to 6.8, but I feel like I'm
missing something:

 - If we have never seen a ".. FILE" line, then (as the changelog notes)
   no files were found to extract feature information from.  In that
   case, why make the self.nestedParse() call at all?  Why not just
   return rather than making a useless call with a random name?

What am I overlooking?

Thanks,

jon

Re: [PATCH 1/8] docs: kernel_feat.py: fix build error for missing files

[Vegard Nossum]

On 06/02/2024 23:53, Jonathan Corbet wrote:
> Vegard Nossum <vegard.nossum@oracle.com> writes:
>> @@ -109,7 +109,7 @@ class KernelFeat(Directive):
>>               else:
>>                   out_lines += line + "\n"
>>   
>> -        nodeList = self.nestedParse(out_lines, fname)
>> +        nodeList = self.nestedParse(out_lines, self.arguments[0])
>>           return nodeList
> 
> So I can certainly track this through to 6.8, but I feel like I'm
> missing something:
> 
>   - If we have never seen a ".. FILE" line, then (as the changelog notes)
>     no files were found to extract feature information from.  In that
>     case, why make the self.nestedParse() call at all?  Why not just
>     return rather than making a useless call with a random name?
> 
> What am I overlooking?

Even if we skip the call in the error/empty case, we still need to pass
a sensible value here in the other cases -- this value is the file that
will be attributed by Sphinx if there is e.g. a reST syntax error in any
of the feature files. 'fname' here is basically the last file that
happened to be read by get_feat.pl, which is more misleading than
self.arguments[0] IMHO.

So basically: Yes, we could skip the call entirely, but we'd still want
to make the above change as well; skipping it doesn't change that.

Maybe we should just change it to doc.current_source directly -- my
rationale for splitting it up into two patches was that we would have
one patch bringing it sort of back to where we were before (having it at
least not error out and still be the obvious/minimal fix that can get
backported to stable) and then a patch actually improving on that (the
next patch in the series).

Does that make sense?


Vegard

Re: [PATCH 1/8] docs: kernel_feat.py: fix build error for missing files

[Jonathan Corbet]

Vegard Nossum <vegard.nossum@oracle.com> writes:

> On 06/02/2024 23:53, Jonathan Corbet wrote:
>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>>> @@ -109,7 +109,7 @@ class KernelFeat(Directive):
>>>               else:
>>>                   out_lines += line + "\n"
>>>   
>>> -        nodeList = self.nestedParse(out_lines, fname)
>>> +        nodeList = self.nestedParse(out_lines, self.arguments[0])
>>>           return nodeList
>> 
>> So I can certainly track this through to 6.8, but I feel like I'm
>> missing something:
>> 
>>   - If we have never seen a ".. FILE" line, then (as the changelog notes)
>>     no files were found to extract feature information from.  In that
>>     case, why make the self.nestedParse() call at all?  Why not just
>>     return rather than making a useless call with a random name?
>> 
>> What am I overlooking?
>
> Even if we skip the call in the error/empty case, we still need to pass
> a sensible value here in the other cases -- this value is the file that
> will be attributed by Sphinx if there is e.g. a reST syntax error in any
> of the feature files. 'fname' here is basically the last file that
> happened to be read by get_feat.pl, which is more misleading than
> self.arguments[0] IMHO.

The purpose is to point the finger at the file that actually contained
the error; are you saying that this isn't working?

Sorry if I'm being slow here,

jon

Re: [PATCH 1/8] docs: kernel_feat.py: fix build error for missing files

[Vegard Nossum]

On 07/02/2024 15:42, Jonathan Corbet wrote:
> Vegard Nossum <vegard.nossum@oracle.com> writes:
> 
>> On 06/02/2024 23:53, Jonathan Corbet wrote:
>>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>>>> @@ -109,7 +109,7 @@ class KernelFeat(Directive):
>>>>                else:
>>>>                    out_lines += line + "\n"
>>>>    
>>>> -        nodeList = self.nestedParse(out_lines, fname)
>>>> +        nodeList = self.nestedParse(out_lines, self.arguments[0])
>>>>            return nodeList
>>>
>>> So I can certainly track this through to 6.8, but I feel like I'm
>>> missing something:
>>>
>>>    - If we have never seen a ".. FILE" line, then (as the changelog notes)
>>>      no files were found to extract feature information from.  In that
>>>      case, why make the self.nestedParse() call at all?  Why not just
>>>      return rather than making a useless call with a random name?
>>>
>>> What am I overlooking?
>>
>> Even if we skip the call in the error/empty case, we still need to pass
>> a sensible value here in the other cases -- this value is the file that
>> will be attributed by Sphinx if there is e.g. a reST syntax error in any
>> of the feature files. 'fname' here is basically the last file that
>> happened to be read by get_feat.pl, which is more misleading than
>> self.arguments[0] IMHO.
> 
> The purpose is to point the finger at the file that actually contained
> the error; are you saying that this isn't working?

For kernel_feat.py: correct, it never did that.

See my longer explanation here:

<https://lore.kernel.org/all/d46018a3-3259-4565-9a25-f9b695519f81@oracle.com/>


Vegard

Re: [PATCH 1/8] docs: kernel_feat.py: fix build error for missing files

[Jonathan Corbet]

Vegard Nossum <vegard.nossum@oracle.com> writes:

>> The purpose is to point the finger at the file that actually contained
>> the error; are you saying that this isn't working?
>
> For kernel_feat.py: correct, it never did that.
>
> See my longer explanation here:
>
> <https://lore.kernel.org/all/d46018a3-3259-4565-9a25-f9b695519f81@oracle.com/>

OK, good enough.  I've applied the patch and will send it Linusward
after a bit of linux-next time.

Thanks,

jon