Received-SPF: pass (google.com: domain of linux-kernel+bounces-44287-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Jani Nikula <jani.nikula@linux.intel.com>
To: Breno Leitao <leitao@debian.org>, corbet@lwn.net, kuba@kernel.org, David
 S. Miller <davem@davemloft.net>
Cc: linux-doc@vger.kernel.org, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, pabeni@redhat.com, edumazet@google.com
Subject: Re: [PATCH v3] Documentation: Document each netlink family
In-Reply-To: <20231121114831.3033560-1-leitao@debian.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <20231121114831.3033560-1-leitao@debian.org>
Date: Tue, 30 Jan 2024 11:43:39 +0200
Message-ID: <874jevjgvo.fsf@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Tue, 21 Nov 2023, Breno Leitao <leitao@debian.org> wrote:
> This is a simple script that parses the Netlink YAML spec files
> (Documentation/netlink/specs/), and generates RST files to be rendered
> in the Network -> Netlink Specification documentation page.

First of all, my boilerplate complaint: All extra processing for Sphinx
should really be done using Sphinx extensions instead of adding Makefile
hacks. I don't think it's sustainable to keep adding this stuff. We
chose Sphinx because it is extensible, and to avoid the Rube Goldberg
machine that the previous documentation build system was.

At the very least I would've expected to see Jon's ack on changes like
this.

The specific problem with this patch, now merged as commit f061c9f7d058
("Documentation: Document each netlink family"), is that it explicitly
writes intermediate files in the $(srctree). Even for O= builds. That's
one of the pitfalls of hacking it in Makefiles.

See below.

> Create a python script that is invoked during 'make htmldocs', reads the
> YAML specs input file and generate the correspondent RST file.
>
> Create a new Documentation/networking/netlink_spec index page, and
> reference each Netlink RST file that was processed above in this main
> index.rst file.
>
> In case of any exception during the parsing, dump the error and skip
> the file.
>
> Do not regenerate the RST files if the input files (YAML) were not
> changed in-between invocations.
>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>

[snip]

> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 2f35793acd2a..5c156fbb6cdf 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -97,7 +97,21 @@ quiet_cmd_sphinx = SPHINX  $@ --> file://$(abspath $(BUILDDIR)/$3/$4)
>  		cp $(if $(patsubst /%,,$(DOCS_CSS)),$(abspath $(srctree)/$(DOCS_CSS)),$(DOCS_CSS)) $(BUILDDIR)/$3/_static/; \
>  	fi
>  
> -htmldocs:
> +YNL_INDEX:=$(srctree)/Documentation/networking/netlink_spec/index.rst
> +YNL_RST_DIR:=$(srctree)/Documentation/networking/netlink_spec
> +YNL_YAML_DIR:=$(srctree)/Documentation/netlink/specs
> +YNL_TOOL:=$(srctree)/tools/net/ynl/ynl-gen-rst.py
> +
> +YNL_RST_FILES_TMP := $(patsubst %.yaml,%.rst,$(wildcard $(YNL_YAML_DIR)/*.yaml))
> +YNL_RST_FILES := $(patsubst $(YNL_YAML_DIR)%,$(YNL_RST_DIR)%, $(YNL_RST_FILES_TMP))
> +
> +$(YNL_INDEX): $(YNL_RST_FILES)
> +	@$(YNL_TOOL) -o $@ -x
> +
> +$(YNL_RST_DIR)/%.rst: $(YNL_YAML_DIR)/%.yaml
> +	@$(YNL_TOOL) -i $< -o $@
> +

Right here.

> +htmldocs: $(YNL_INDEX)
>  	@$(srctree)/scripts/sphinx-pre-install --version-check
>  	@+$(foreach var,$(SPHINXDIRS),$(call loop_cmd,sphinx,html,$(var),,$(var)))
>  
> diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
> index 683eb42309cc..cb435c141794 100644
> --- a/Documentation/networking/index.rst
> +++ b/Documentation/networking/index.rst
> @@ -55,6 +55,7 @@ Contents:
>     filter
>     generic-hdlc
>     generic_netlink
> +   netlink_spec/index
>     gen_stats
>     gtp
>     ila
> diff --git a/Documentation/networking/netlink_spec/.gitignore b/Documentation/networking/netlink_spec/.gitignore
> new file mode 100644
> index 000000000000..30d85567b592
> --- /dev/null
> +++ b/Documentation/networking/netlink_spec/.gitignore
> @@ -0,0 +1 @@
> +*.rst

And then goes on to git ignore the mess it made.


BR,
Jani.


-- 
Jani Nikula, Intel