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
