[bpf-next,v4,1/2] Add subdir support to Documentation makefile

Commit Message

Donald Hunter Sept. 22, 2022, 11:52 a.m. UTC

Run make in list of subdirs to build generated sources and migrate
userspace-api/media to use this instead of being a special case.

Signed-off-by: Donald Hunter <[e-mail]>
---
 Documentation/Makefile                     | 15 +++++++++++++--
 Documentation/userspace-api/media/Makefile |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

Comments

Jonathan Corbet Sept. 22, 2022, 1:24 p.m. UTC | #1

Donald Hunter <[e-mail]> writes:

> Run make in list of subdirs to build generated sources and migrate
> userspace-api/media to use this instead of being a special case.
>
> Signed-off-by: Donald Hunter <[e-mail]>

This could really use a bit more information on exactly what you're
doing and why you want to do it.

Beyond that, I would *really* like to see more use of Sphinx extensions
for this kind of special-case build rather than hacking in more
special-purpose scripts.  Is there a reason why it couldn't be done that
way?

Thanks,

jon

Donald Hunter Sept. 23, 2022, 2:58 p.m. UTC | #2

Jonathan Corbet <[e-mail]> writes:

> Donald Hunter <[e-mail]> writes:
>
>> Run make in list of subdirs to build generated sources and migrate
>> userspace-api/media to use this instead of being a special case.
>>
>> Signed-off-by: Donald Hunter <[e-mail]>
>
> This could really use a bit more information on exactly what you're
> doing and why you want to do it.

What would you like me to add? Something like:

"... in preparation for running a generator script to produce .csv data
used by bpf documentation"

> Beyond that, I would *really* like to see more use of Sphinx extensions
> for this kind of special-case build rather than hacking in more
> special-purpose scripts.  Is there a reason why it couldn't be done that
> way?

I looked at writing the BPF program types as a Sphinx extension but
found that approach problematic for the following reasons:

- This needs to run both in the kernel tree and the libbpf Github
  project. The tree layouts are different so the relative paths to
  source files are different. I don't see an elegant way to handle this
  inline in a .rst file. This can easily be handled in Makefiles
  that are specific to each project.

- It makes use of csv-table which does all the heavy lifting to produce
  the desired output.

- I have zero experience of extending Sphinx.

I thought about submitting this directly to the libbpf Github project
and then just linking from the kernel docs to the page about program
types in the libbpf project docs. But I think it is preferable to master
the gen-bpf-progtypes.sh script in the kernel tree where it can be
maintained in the same repo as the libbpf.c source file it parses.

Donald Hunter Sept. 30, 2022, 9:57 a.m. UTC | #3

Donald Hunter <[e-mail]> writes:

> Jonathan Corbet <[e-mail]> writes:
>
>> Beyond that, I would *really* like to see more use of Sphinx extensions
>> for this kind of special-case build rather than hacking in more
>> special-purpose scripts.  Is there a reason why it couldn't be done that
>> way?
>
> I looked at writing the BPF program types as a Sphinx extension but
> found that approach problematic for the following reasons:
>
> - This needs to run both in the kernel tree and the libbpf Github
>   project. The tree layouts are different so the relative paths to
>   source files are different. I don't see an elegant way to handle this
>   inline in a .rst file. This can easily be handled in Makefiles
>   that are specific to each project.
>
> - It makes use of csv-table which does all the heavy lifting to produce
>   the desired output.
>
> - I have zero experience of extending Sphinx.
>
> I thought about submitting this directly to the libbpf Github project
> and then just linking from the kernel docs to the page about program
> types in the libbpf project docs. But I think it is preferable to master
> the gen-bpf-progtypes.sh script in the kernel tree where it can be
> maintained in the same repo as the libbpf.c source file it parses.

Given the pushback on Makefile changes and the need for this patch to be
compatible with both the kernel tree and the libbpf repo, can I suggest
a pragmatic way forward.

I suggest that I drop the gen-bpf-progtypes.sh script and Makefile
changes from the patchset and just submit static documentation contents
for the table of BPF program types. This would avoid any downstream
breakage when syncing from the kernel tree to the libbpf github
repo. The table of BPF program types can be maintained manually which
should not be a burden going forward. Another benefit would be that the
resulting documentation can be curated more easily than if it were
auto-generated.

Andrii Nakryiko Sept. 30, 2022, 10:25 p.m. UTC | #4

On Fri, Sep 30, 2022 at 2:58 AM Donald Hunter <[e-mail]> wrote:
>
> Donald Hunter <[e-mail]> writes:
>
> > Jonathan Corbet <[e-mail]> writes:
> >
> >> Beyond that, I would *really* like to see more use of Sphinx extensions
> >> for this kind of special-case build rather than hacking in more
> >> special-purpose scripts.  Is there a reason why it couldn't be done that
> >> way?
> >
> > I looked at writing the BPF program types as a Sphinx extension but
> > found that approach problematic for the following reasons:
> >
> > - This needs to run both in the kernel tree and the libbpf Github
> >   project. The tree layouts are different so the relative paths to
> >   source files are different. I don't see an elegant way to handle this
> >   inline in a .rst file. This can easily be handled in Makefiles
> >   that are specific to each project.
> >
> > - It makes use of csv-table which does all the heavy lifting to produce
> >   the desired output.
> >
> > - I have zero experience of extending Sphinx.
> >
> > I thought about submitting this directly to the libbpf Github project
> > and then just linking from the kernel docs to the page about program
> > types in the libbpf project docs. But I think it is preferable to master
> > the gen-bpf-progtypes.sh script in the kernel tree where it can be
> > maintained in the same repo as the libbpf.c source file it parses.
>
> Given the pushback on Makefile changes and the need for this patch to be
> compatible with both the kernel tree and the libbpf repo, can I suggest
> a pragmatic way forward.
>
> I suggest that I drop the gen-bpf-progtypes.sh script and Makefile

It's way too easy to forget to update this table when adding new
program type support in libbpf. So if possible I think script is the
way to go.

Jonathan, given the script is really minimal and allows to keep
documentation in sync with libbpf source code, do you have a strong
objection? Writing a custom plugin seems like a too high bar for
something pretty straightforward like this?

Also, should this be routed through your tree or you'd like us to take
it through bpf-next tree?


Donald, if Jonathan is feeling really strongly, then I guess manually
generated table is ok approach as well, but let's hear from Jonathan
first.

Also cc Grant about logistics of kernel vs libbpf docs.


> changes from the patchset and just submit static documentation contents
> for the table of BPF program types. This would avoid any downstream
> breakage when syncing from the kernel tree to the libbpf github
> repo. The table of BPF program types can be maintained manually which
> should not be a burden going forward. Another benefit would be that the
> resulting documentation can be curated more easily than if it were
> auto-generated.

Patch

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 64d44c1ecad3..8a63ef2dcd1c 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -65,6 +65,12 @@  I18NSPHINXOPTS  = $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) .
 # commands; the 'cmd' from scripts/Kbuild.include is not *loopable*
 loop_cmd = $(echo-cmd) $(cmd_$(1)) || exit;
 
+BUILD_SUBDIRS = \
+	Documentation/userspace-api/media
+
+quiet_cmd_build_subdir = SUBDIR  $2
+      cmd_build_subdir = $(MAKE) BUILDDIR=$(abspath $(BUILDDIR)) $(build)=$2 $3
+
 # $2 sphinx builder e.g. "html"
 # $3 name of the build subfolder / e.g. "userspace-api/media", used as:
 #    * dest folder relative to $(BUILDDIR) and
@@ -74,7 +80,7 @@  loop_cmd = $(echo-cmd) $(cmd_$(1)) || exit;
 #    e.g. "userspace-api/media" for the linux-tv book-set at ./Documentation/userspace-api/media
 
 quiet_cmd_sphinx = SPHINX  $@ --> file://$(abspath $(BUILDDIR)/$3/$4)
-      cmd_sphinx = $(MAKE) BUILDDIR=$(abspath $(BUILDDIR)) $(build)=Documentation/userspace-api/media $2 && \
+      cmd_sphinx =  \
 	PYTHONDONTWRITEBYTECODE=1 \
 	BUILDDIR=$(abspath $(BUILDDIR)) SPHINX_CONF=$(abspath $(srctree)/$(src)/$5/$(SPHINX_CONF)) \
 	$(PYTHON3) $(srctree)/scripts/jobserver-exec \
@@ -93,6 +99,7 @@  quiet_cmd_sphinx = SPHINX  $@ --> file://$(abspath $(BUILDDIR)/$3/$4)
 
 htmldocs:
 	@$(srctree)/scripts/sphinx-pre-install --version-check
+	@+$(foreach var,$(BUILD_SUBDIRS),$(call loop_cmd,build_subdir,$(var),html))
 	@+$(foreach var,$(SPHINXDIRS),$(call loop_cmd,sphinx,html,$(var),,$(var)))
 
 linkcheckdocs:
@@ -100,6 +107,7 @@  linkcheckdocs:
 
 latexdocs:
 	@$(srctree)/scripts/sphinx-pre-install --version-check
+	@+$(foreach var,$(BUILD_SUBDIRS),$(call loop_cmd,build_subdir,$(var),latex))
 	@+$(foreach var,$(SPHINXDIRS),$(call loop_cmd,sphinx,latex,$(var),latex,$(var)))
 
 ifeq ($(HAVE_PDFLATEX),0)
@@ -112,6 +120,7 @@  else # HAVE_PDFLATEX
 
 pdfdocs: latexdocs
 	@$(srctree)/scripts/sphinx-pre-install --version-check
+	@+$(foreach var,$(BUILD_SUBDIRS),$(call loop_cmd,build_subdir,$(var),latex))
 	$(foreach var,$(SPHINXDIRS), \
 	   $(MAKE) PDFLATEX="$(PDFLATEX)" LATEXOPTS="$(LATEXOPTS)" -C $(BUILDDIR)/$(var)/latex || exit; \
 	   mkdir -p $(BUILDDIR)/$(var)/pdf; \
@@ -122,10 +131,12 @@  endif # HAVE_PDFLATEX
 
 epubdocs:
 	@$(srctree)/scripts/sphinx-pre-install --version-check
+	@+$(foreach var,$(BUILD_SUBDIRS),$(call loop_cmd,build_subdir,$(var),epub))
 	@+$(foreach var,$(SPHINXDIRS),$(call loop_cmd,sphinx,epub,$(var),epub,$(var)))
 
 xmldocs:
 	@$(srctree)/scripts/sphinx-pre-install --version-check
+	@+$(foreach var,$(BUILD_SUBDIRS),$(call loop_cmd,build_subdir,$(var),xml))
 	@+$(foreach var,$(SPHINXDIRS),$(call loop_cmd,sphinx,xml,$(var),xml,$(var)))
 
 endif # HAVE_SPHINX
@@ -138,7 +149,7 @@  refcheckdocs:
 
 cleandocs:
 	$(Q)rm -rf $(BUILDDIR)
-	$(Q)$(MAKE) BUILDDIR=$(abspath $(BUILDDIR)) $(build)=Documentation/userspace-api/media clean
+	@+$(foreach var,$(BUILD_SUBDIRS),$(call loop_cmd,build_subdir,$(var),clean))
 
 dochelp:
 	@echo  ' Linux kernel internal documentation in different formats from ReST:'
diff --git a/Documentation/userspace-api/media/Makefile b/Documentation/userspace-api/media/Makefile
index 00922aa7efde..783c6f880b72 100644
--- a/Documentation/userspace-api/media/Makefile
+++ b/Documentation/userspace-api/media/Makefile
@@ -50,6 +50,8 @@  $(BUILDDIR)/lirc.h.rst: ${UAPI}/lirc.h ${PARSER} $(SRC_DIR)/lirc.h.rst.exception
 .PHONY: all html epub xml latex
 
 all: $(IMGDOT) $(BUILDDIR) ${TARGETS}
+	@:
+
 html: all
 epub: all
 xml: all