Message-ID: <20250214032457.6444ee93@foz.lan>
Date: Fri, 14 Feb 2025 03:24:57 +0100
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Jonathan Corbet <corbet@....net>
Cc: Linux Doc Mailing List <linux-doc@...r.kernel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scripts/kernel-doc: remove an obscure logic from
 kernel-doc

Em Thu, 13 Feb 2025 09:35:58 -0700
Jonathan Corbet <corbet@....net> escreveu:

> Mauro Carvalho Chehab <mchehab+huawei@...nel.org> writes:
> 
> > Kernel-doc has an obscure logic that uses an external file
> > to map files via a .tmp_filelist.txt file stored at the current
> > directory. The rationale for such code predates git time,
> > as it was added on Kernel v2.4.5.5, with the following description:
> >
> > 	# 26/05/2001 -         Support for separate source and object trees.
> > 	#              Return error code.
> > 	#              Keith Owens <kaos@....com.au>
> >
> > from commit 396a6123577d ("v2.4.5.4 -> v2.4.5.5") at the historic
> > tree:
> > 	https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
> >
> > Support for separate source and object trees is now done on a different
> > way via make O=<object>.
> >
> > There's no logic to create such file, so it sounds to me that this is
> > just dead code.
> >
> > So, drop it.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > ---
> >  scripts/kernel-doc | 19 +------------------
> >  1 file changed, 1 insertion(+), 18 deletions(-)  
> 
> Weird ... I went and looked, and can't find anything that ever created
> that tmp_filelist.txt file; I wonder if this code ever did anything?

I wonder the same ;-) Anyway, better to remove this now, as, if people
complain, it would be easier to revert than after switching to the
Python version.

> Don't put that functionality into the Python version :)

Yeah, I started implementing it, but it sounded a waste of time, so
I dropped it from the RFC versions. It sounded too complex for people
to maintain a separate tmp file when make O=dir would do it on a much
better and automated way.

-

With regards to the Python transition, since our Makefile allows
switching to a different script since ever[1], I'm playing with 
the idea of sending a patch series with:

Patch 1: 
  - drops Sphinx version check from both kerneldoc 
    (-sphinx-version parameter) and the corresponding Sphinx extension;

patch 2: 
  - renames kerneldoc to kerneldoc.pl
  - creates a symlink:
	kerneldoc.pl -> kerneldoc

patch 3:
  - adds kerneldoc.py:

patch 4:
  - add info messages on both versions related to the transition,
    and instructions about using KERNELDOC=<script> makefile and ask
    people to report eventual regressions with new script.

patch 5:
  - change kerneldoc symlink to point to kerneldoc.py

We can then keep both for maybe one Kernel cycle and see how it goes,
stop accepting patches to the Perl version, in favor of doing the needed
changes at the Python one.

If everything goes well, we can remove the venerable Perl version on the 
upcoming merge window, and change the Sphinx extension to use the Python
classes directly instead of running an external executable code.

What do you think?

-

I'm in doubt if I should split the Kernel classes for the Python version
into a scripts/lib/kdoc directory on this series or doing such change
only after we drop the Perl version.

Keeping it on a single file helps to do more complex code adjustments 
on a single place, specially if we end renaming/shifting stuff[2].

[1] I didn't remember about that - it is a very welcomed feature,
    probably thanks to Markus.

[2] the currently global const regex macros is something that I want
    to rename and place them inside a class (or on multiple classes).
    Also, Python coding style is to use uppercase for const. There is
    currently a Pylint disabled warning about that. So, I do plan to
    do such changes in the future to make it more compatible with
    Python coding style.

-

On a separate but related issue, perhaps we should start talking about
coding style. We don't have anything defined at the Kernel, and
different scripts follow different conventions (or most likely
don't follow any convention at all). We should probably think having 
something defined in the future.

From my side, I like Pylint warnings, except for the (IMHO) useless "Rxxx"
warnings that complains about too many/too few things. They have
been useful to detect hidden bugs at scripts, and it allows inlined
exceptions to the coding style.

autopep8 autoformatter is also nice (and, up to some point, black),
as it auto-corrects whitespace issues, but there's two things I don't 
like on them:

1. its coding style on lines, creating function calls with open 
   parenthesis:

	-    parser.add_argument("-n", "-nosymbol", "--nosymbol", action='append',
	-                         help=NOSYMBOL_DESC)
	+    parser.add_argument(
	+        "-n", "-nosymbol", "--nosymbol", action="append", help=NOSYMBOL_DESC
	+    )

2. whitespace removal on aligned consts:

	-    STATE_INLINE_NA     = 0 # not applicable ($state != STATE_INLINE)
	-    STATE_INLINE_NAME   = 1 # looking for member name (@foo:)
	-    STATE_INLINE_TEXT   = 2 # looking for member documentation
	-    STATE_INLINE_END    = 3 # done
	-    STATE_INLINE_ERROR  = 4 # error - Comment without header was found.
	-                            # Spit a warning as it's not
	-                            # proper kernel-doc and ignore the rest.
	+    STATE_INLINE_NA = 0  # not applicable ($state != STATE_INLINE)
	+    STATE_INLINE_NAME = 1  # looking for member name (@foo:)
	+    STATE_INLINE_TEXT = 2  # looking for member documentation	
	+    STATE_INLINE_END = 3  # done
	+    STATE_INLINE_ERROR = 4  # error - Comment without header was found.

What I do here is from time to time manually run them and cherry-pick
only changes that sounds good to my personal taste.

It is probably a good idea to define a coding style and perhaps add
some config files (like a .pep8 file) to have a single coding style for
future code, letting scripts/checkpatch.pl to run pylint and/or some
other coding style tool(s).

Thanks,
Mauro

Powered by blists - more mailing lists