Re: [RFC v2 18/38] docs: sphinx/kernel_abi: use AbiParser directly

[Mauro Carvalho Chehab]

Hi Jon,

Em Wed, 29 Jan 2025 01:43:24 +0100
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> escreveu:

> Em Tue, 28 Jan 2025 15:37:25 -0700
> Jonathan Corbet <corbet@xxxxxxx> escreveu:
> 
> > Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> writes:
> >   
> > > Instead of running get_abi.py script, import AbiParser class and
> > > handle messages directly there using an interactor. This shold save some
> > > memory, as there's no need to exec python inside the Sphinx python
> > > extension.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > > ---
> > >  Documentation/sphinx/kernel_abi.py | 26 +++++++++++++++-----------
> > >  scripts/get_abi.py                 |  2 +-
> > >  2 files changed, 16 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/Documentation/sphinx/kernel_abi.py b/Documentation/sphinx/kernel_abi.py
> > > index fc7500fad119..93d537d8cb6c 100644
> > > --- a/Documentation/sphinx/kernel_abi.py
> > > +++ b/Documentation/sphinx/kernel_abi.py
> > > @@ -42,6 +42,11 @@ from docutils.parsers.rst import directives, Directive
> > >  from sphinx.util.docutils import switch_source_input
> > >  from sphinx.util import logging
> > >  
> > > +srctree = os.path.abspath(os.environ["srctree"])
> > > +sys.path.insert(0, os.path.join(srctree, "scripts"))
> > > +
> > > +from get_abi import AbiParser    
> > 
> > I have to admit that I don't like this bit of messing around with the
> > path.  And importing things out of scripts/ seems ... inelegant.
> > 
> > I take it you still want to be able to run get_abi.py standalone even
> > after it's directly integrated into Sphinx?    
> 
> Yes, because calling it via command line provides:
> 
> 1. a way to test the parser and check the results;
> 2. a search utility;
> 3. the undefined symbol verification.
> 
> Btw, if you look at the other Sphinx modules, they do exactly the same:
> they execute code from scripts/. The only difference here is that,
> instead of loading the a perl/python/shell engine and running the entire
> script from there, it is importing just a class.
> 
> > In this case, it might be
> > nicer to have the common library functionality in its own module that
> > can be imported into both sphinx and the standalone command.   
> 
> This would be possible too: place the classes on a common lib dir and
> then import it from both command line and Sphinx extensions.
> 
> If we're willing to do that, then perhaps we can have separate files
> for each different class, as this could make it easier to maintain.
> 
> > That still
> > leaves open the question of where that module lives
> > (Documentation/sphinx perhaps?) and how the Python path gets set up
> > correctly...  
> 
> I guess the command line at scripts/ could use something like this to
> get the library location (untested) and add to the import search PATH:
> 
> 	import os
> 
> 	python_lib_dir="some/location"
> 
> 	scriptdir = os.path.dirname(os.path.realpath(__file__))
> 
> 	sys.path.insert(0, os.path.join(srctree, f"../{python_lib_dir}"))
> 
> 	from abi_parser import abiParser
> 
> Now, I'm not sure if the best location for python libraries would
> be at Documentation/sphinx, as we may end needing other python
> libraries with time and not all would be used by Sphinx.
> 
> In short: I would be more inclined to place them on 
> a new lib directory (tools/lib? tools/py_lib? scripts/lib?).
> 
> See, with the content of this series, if we split files per each class,
> it would mean 3 files:
> 
>   1. abi_parser.py, containing AbiParser class (plus ABI_DIR const);
>      (this is the only class used by Documentation/sphinx extensions)
>   2. abi_regex.py, containing AbiRegex class;
>   3. abi_symbols.py, containing SystemSymbols class.
> 
> Now, if we're going on this direction, it may also make sense to split
> the command line classes/functions into 4 (or 5 files) for argparse
> argument definition and command run code. If we do that, it means that
> other files will be stored somewhere:
> 
>   4. abi_cmd_rest.py: AbiRest and AbiValidate classes for the rest 
>      and validate arguments (I would likely place both at the same file,
>      as the code is similar - but it could also be split on two separate
>      files);
>   5. abi_cmd_search.py: AbiSearch - for the search arguments;
>   6. abi_cmd_undefined.py: AbiUndefined - for the undocumented symbol check 
>      arguments;
> 
> Finally, there is the one under scripts/:
> 
>   7. get_abi.py: with the main function
> 
> For (1), Documentation/sphinx could make sense, but (2) to (6) are
> used only by the command line tool. Placing them at Documentation/ 
> seems weird. Well, nothing prevents having them at scripts/, IMHO, things
> would become more organized if we place the Python files with 0644
> flags elsewhere.

As I'll be preparing such patches for merge along this week, I'd
like to know what do you prefer in terms of directories:

1. Keep it as-is;
2. have a separate library directory for Python modules
   (scripts/lib?);
3. place python modules inside scripts/;
4. place python modules inside Documentation/sphinx (IMO a bad
   idea);
5. something else

-

Btw, I'm considering to also submit later a patchset similar to
this one converting kernel-doc to Python. I already started writing
something like that (written from the scratch, following as much
as possible what we have today on Perl to avoid regressions).

I would probably split the code into separate classes to make the code 
more readable/maintainable (a base class, a class with rest output,
another one with man output, and a few other helper classes).

Thanks,
Mauro