Message-ID: <hjooonedrtoykg6qgg7kvat2ovtv6dtr3ulih6nowkds4teyuc@khetujrxwugv>
Date: Fri, 22 Aug 2025 15:55:16 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: Jonathan Corbet <corbet@....net>, 
	Linux Doc Mailing List <linux-doc@...r.kernel.org>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Alex Gaynor <alex.gaynor@...il.com>, Alice Ryhl <aliceryhl@...gle.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Trevor Gross <tmgross@...ch.edu>, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 04/11] scripts: sphinx-build-wrapper: add a wrapper for
 sphinx-build

On Fri, Aug 22, 2025 at 04:06:45AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 21 Aug 2025 14:11:06 -0600
> Jonathan Corbet <corbet@....net> escreveu:
> 
> > Mauro Carvalho Chehab <mchehab+huawei@...nel.org> writes:
> > 
> > > There are too much magic inside docs Makefile to properly run
> > > sphinx-build. Create an ancillary script that contains all
> > > kernel-related sphinx-build call logic currently at Makefile.
> > >
> > > Such script is designed to work both as an standalone command
> > > and as part of a Makefile. As such, it properly handles POSIX
> > > jobserver used by GNU make.
> > >
> > > It should be noticed that, when running the script alone,
> > > it will only take care of sphinx-build and cleandocs target.
> > > As such:
> > >
> > > - it won't run "make rustdoc";
> > > - no extra checks.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > > ---
> > >  .pylintrc                    |   2 +-
> > >  scripts/sphinx-build-wrapper | 627 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 628 insertions(+), 1 deletion(-)
> > >  create mode 100755 scripts/sphinx-build-wrapper  
> > 
> > As a whole I like the idea of this - I would rather be reading code in
> > Python than in makefilese.  But I have some overall notes...
> > 
> > I am a bit dismayed by the size of it; this is many times the amount of
> > code it allows us to remove from the makefile.  Perhaps there's nothing
> > to be done for that, but ...
> 
> True:
> 
>  7 files changed, 922 insertions(+), 200 deletions(-)
> 
> Yet, a lot of them are blank lines, comments, docstrings and 
> argparse. Also, there are some improvements on it, like the PDF
> build error handling logic (more below).
> 
> One of the things I wanted to do is precisely better describe
> what it does. The Makefile has lots of magic and complex macros
> that, even the ones that wrote it have headaches trying to understand
> after some time not looking on it.
> 
> Also, part of it will be dropped at the next patch series when
> we will get rid of userspace-api/media/Makefile:
> 
> 	$ git diff 182fa089db0b..parse_headers scripts/sphinx-build-wrapper|diffstat -p1 
> 	 scripts/sphinx-build-wrapper |   48 ---------------------
> 	 1 file changed, 48 deletions(-)

After sleeping on it, IMO the best would be if we change the order:
Let's place the series related to parse-headers.pl/kernel-images.py
first. 

Then, on this series, I can suppress the code that has support for
the media build Makefile (reducing those 48 lines), making the
patches cleaner.

I'll also change this series to place the new wrapper under
tools/docs, splitting part of it on a library. The final code
will be bigger due to the library, but it will become more
modular and with a cleaner implementation.

> 
> I guess there are some space to clean it up a little bit, for instance
> getting rid of most of the env vars and passing them as command
> line only. Yet, I opted to be a bit conservative at the cleanups at the
> env vars to reduce the risk of breaking something.
> 
> > Is there value in the SphinxBuilder class?  Just because you can create
> > classes in Python doesn't mean that you have to; I'm not sure why you
> > would create one here rather than just doing it all at the module level.
> 
> On Python, I prefer using classes, as:
> 
> 1. I don't like passing lots of arguments to functions;
> 2. Python doesn't have struct. The closest concept of struct in Python
>    is a data class;
> 3. Classes help to avoid global vars. In this specific case, the size of 
>    its data "struct" (e.g. class variables) is not small, as it has to
>    parse lots of environment vars, and several of those are used on
>    multiple places.
> 
> > Is the "search for a newer Python" code really going to be useful for
> > anybody? 
> 
> Well, I could have written it in shell script or Perl ;-)
> 
> Seriously, the rationale was not to search for a newer Python code, but
> instead, to have a place were it is easier to understand what's
> going on and adjust to our actual needs.
> 
> I actually started it because of the pdfdocs check: doing make
> pdfdocs currently will return an error code when building docs
> no matter what. Fixing it inside the Makefile would be doable, but
> complex and would likely require an script anyway (or a script-like
> code embedded on it).
> 
> Now, instead of adding yet another hack there, why not do it
> right?
> 
> > It seems like a lot of work (and code) to try to quietly patch
> > things up for somebody who has some sort of a strange setup.
> 
> My idea was not to support some strange setup, but to I had a few
> goals. Mainly:
> 
> 1. to have the simplest possible Makefile without loosing anything;
> 2. to be able to call the script directly, as it helps debugging;
> 3. to remove that ugly for sphinx-build call macro logic inside
>    Makefile, with is hard to understand and even harder to touch;
> 4. to fix a known bug with the current approach with regards
>    to PDF build: no matter if PDF build succeeded or not, it
>    always return an error code;
> 5. to have a summary of the PDF build. Even with latexmk, the
>    PDF build is a way too noisy, being hard to check what broke
>    and what succeeded.
> 6. to make easier to build PDFs in interactive mode (I added this
>    later at the development cycle).
> 
> -
> 
> For the future, if time permits, there are two possible improvements
> 
> a) I'd like to change the way SPHINXDIRS work. Right now, it is a very
>    dirty hack, that makes sphinx-build becalled several times (one for
>    each dir), and breaks cross-references.
> 
>    I'd like to be able to allow building multiple dirs at the
>   same time, with a proper index between them.
> 
> b) By having this in python, we can do other cleanups like:
> 
>    - instance convert sphinx-pre-install into a class, calling
>      it directly there;
>    - pick the logic inside conf.py that handles SPHINXDIRS and
>      latex documents.
> 
> In summary, the advantage is that it is a lot easier to fine tune
> the script in the proper way than to add more hacks to docs Makefile.
> 
> > Please, no "except Exception:"  (or the equivalent bare "except:").
> 
> Ok.
> 
> > That bit of locale tweaking shows up in enough places that it should
> > maybe go into a little helper module rather than being repeatedly
> > open-coded?
> 
> Do you mean this?
> 
>         # The sphinx-build tool has a bug: internally, it tries to set
>         # locale with locale.setlocale(locale.LC_ALL, ''). This causes a
>         # crash if language is not set. Detect and fix it.
>         try:
>             locale.setlocale(locale.LC_ALL, '')
>         except Exception:		# I'll replace it with locale.Error
>             self.env["LC_ALL"] = "C"
>             self.env["LANG"] = "C"
> 
> This was also added later, to fix a current Sphinx (or Python?) bug.
> 
> Try to run sphinx-build on a Debian-based or Gentoo distro without 
> setting any locale (which is the default after installing them):
> 
> 	# sphinx-build --help
> 	Traceback (most recent call last):
> 	  File "/usr/bin/sphinx-build", line 8, in <module>
> 	    sys.exit(main())
> 	             ~~~~^^
> 	  File "/usr/lib/python3/dist-packages/sphinx/cmd/build.py", line 546, in main
> 	    locale.setlocale(locale.LC_ALL, '')
> 	    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
> 	  File "/usr/lib/python3.13/locale.py", line 615, in setlocale
> 	    return _setlocale(category, locale)
> 	locale.Error: unsupported locale setting
> 
> 	# LC_ALL=C sphinx-build --help
> 	usage: sphinx-build [OPTIONS] SOURCEDIR OUTPUTDIR [FILENAMES...]
> 	...
> 
> 
> Well, sphinx-build will crash even if called with "--help". Btw, 
> spinx-build is following Python recommendation of using '':
> 	https://docs.python.org/3/library/locale.html#locale.setlocale
> 
> Ok, we could drop that, but on the other hand it is just 5 LOC.
> (actually it can be 4 LOC, as self.env["LANG"] = "C" is not really needed 
> to avoid the crash).
> 
> One thing I'm in doubt is with regards to --venv command line argument.
> It could be dropped, or it could be auto-detected. The code is also
> small, and IMHO it could help for the ones using venv:
> 
>         # If venv parameter is specified, run Sphinx from venv
>         if venv:
>             bin_dir = os.path.join(venv, "bin")
>             if os.path.isfile(os.path.join(bin_dir, "activate")):
>                 # "activate" virtual env
>                 self.env["PATH"] = bin_dir + ":" + self.env["PATH"]
>                 self.env["VIRTUAL_ENV"] = venv
>                 if "PYTHONHOME" in self.env:
>                     del self.env["PYTHONHOME"]
>                 print(f"Setting venv to {venv}")
>             else:
>                 sys.exit(f"Venv {venv} not found.")
> 
> Btw, this is the kind of code that makes sense to be inside some
> library.
> 
> Thanks,
> Mauro

-- 
Thanks,
Mauro

Powered by blists - more mailing lists