docs: Update dependencies for documentation build system (Sphinx, add-ons etc.) #24014
Conversation
qmonnet
added
area/documentation
qmonnet
force-pushed
the
pr/docs-framework-updates
branch
from
ea21925
to
cd5e400
Compare
The Sphinx plug-in for Google Analytics has been added to the full list of runtime dependencies for the build system for the documentation, but never to the subset list from which the full one is generated. As a consequence, regenerating the full list with the related command, "make -C Documentation update-requirements", removes this dependency. Add the plug-in to the minimal list so that we don't lose it by accident. Fixes: cilium#22821 Fixes: 4bc5629 ("Add sphinxcontrib-googleanalytics to doc requirements") Signed-off-by: Quentin Monnet <quentin@isovalent.com>
The table should be indented with the item list it belongs to, or Sphinx will believe the list is finished and open a new one, under the table, starting from 3. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
There are two target references (non-anonymous links) names "netperf", and pointing to the same link anyway. Let's clean it up. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
To make sure there are no spelling errors when building the docs, we look at the output directory for the spell checker, with "ls". But if the directory has not been created, for example because no spelling mistake has been found, this may pring an error in the user's terminal. Make sure we discard any error from this "ls" command. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Dependabot has updated the version for rstcheck in Documentation/requirements-min/requirements.txt, but it has never been synchronised with Documentation/requirements.txt after that. And as it happens, rstcheck 6.1.1 has had several breaking changes since 3.3.1, and a few updates are necessary: - Option --report has been replaced with --report-level. - Option --ignore-language has been replaced with --ignore-languages. - Option --ignore-message has been replaced with --ignore-messages. - For some reason, the ..openapi directive is now reported as an issue. - It seems that rstcheck is better at analysing code blocks, but this results in many reports for our C blocks (which are not meant to be parsed as standalone snippets), so we skip analysis of C blocks. - But the issue with malformed tables has been fixed and we can remove the ignore on the relevant message. At last, the most annoying thing is that this version has a bug, showing "critical" level messages about AttributeError reports. It mentions that it may be related to code blocks without a language specified, but we don't have these (or are not supposed to), and other people have reported this might also be caused by directives in included text fragments. See the relevant GitHub issue for more details: rstcheck/rstcheck-core#3. At least, these messages do not cause rstcheck to return with an error, so we can deal with them by simply filtering them out. New reports from rstcheck 6.1.1 that we do not want to ignore have been fixed in previous commits. Fixes: cilium#22155 Fixes: a819d97 ("build(deps): bump rstcheck in /Documentation/requirements-min") Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Update all that we can in Documentation/requirements-min/requirements.txt, then run the command "make update-requirements" to update all dependencies accordingly. There are some dependencies that we cannot update to their latest version: - Sphinx itself cannot be updated to 6+, because myth-parser does not support it. - sphinx-tabs cannot be updated from 3.4.0 to 3.4.1, because the latter needs docutils 0.18 which is not currently supported by our sphinx_rtd_theme fork (it would need a rebase). Signed-off-by: Quentin Monnet <quentin@isovalent.com>
qmonnet
force-pushed
the
pr/docs-framework-updates
branch
from
cd5e400
to
3b74687
Compare
|
Testing the changes: We need a new docs-builder image to properly test the changes. I built one, and pointed to it in a new (temporary) commit to make sure the CI workflow passes. It works (note the use of
The Netlify previews builds fine as well. @joestringer, assuming the PR looks good to you, could you please pull the changes and push a new Other than this pointer to the final image, the PR is ready for review. [EDIT] Oh no wait I'll also get rid of the Dependabot updates. |
qmonnet
force-pushed
the
pr/docs-framework-updates
branch
from
3b74687
to
15a50f5
Compare
|
Added a commit to remove the Dependabot configuration for updates. We should no longer get PRs to updates Documentation/requirements-min/requirements.txt (we never merge them directly anyway), but should still get updates on Documentation/requirements.txt directly when Dependabot has a security advisory. |
There was a problem hiding this comment.
LGTM except for generating the new docs-builder image.
Seems like there's a similar issue with the docs-builder here as we have with the cilium-builder / cilium-runtime images. I agree for now it's OK to just handle this manually. If this bothers you, I would suggest looking a bit closer into (a) the way that the images/ infrastructure detects changes and figures out canonical shas for a given set of input files, (b) the bot that can automatically build an image and update the commits in-line. Andre and I can help with any necessary docker repository configurations etc.
|
If you push this as a PR directly to cilium/cilium, then it's a little easier to generate the image - effectively what I'm doing on the other side is just logging into dockerhub and then initiating a docs-builder image build from the dockerhub infrastructure. I think that previous times I have just copied this branch to the main repo in order to do this. |
|
(I can take care of this for now, so you don't have to reopen the PR) |
qmonnet
force-pushed
the
pr/docs-framework-updates
branch
from
15a50f5
to
432a6e8
Compare
Following the recent updates to the docs components, in particular the version bump for rstcheck, a new docs-builder image is necessary. Otherwise, rstcheck in the old image will not recognising the new syntax for its arguments, and the workflow will fail. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Remove Dependabot's updates for required dependencies.
The reason is that when Dependabot attempts to update
Documentation/requirements-min/requirements.txt, then it is not enough
to merge this PR. Instead, we have to synchronise with the actual list
of dependencies ("make -C Documentation update-requirements"), and to
check that everything still works as intended.
Most of the time, Dependabot's updates for documentation requirements
are just noise, because the update don't bring any useful feature, and
we don't have the cycles to sync and test the changes for each PR, this
is simply too much work for too little benefit.
Worse, some PRs might be merged without taking the time to sync and
test, for example: cilium#22155, and in
such case further updates of the full requirements list will be broken.
So let's stick to manual updates again.
Regarding security updates, it appears that Dependabot offers them
regardless of this configuration file (by updating directly
Documentation/requirements.txt), as seen in
cilium#23980.
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
qmonnet
force-pushed
the
pr/docs-framework-updates
branch
from
432a6e8
to
4a7bf37
Compare
I agree the current process is not ideal. I didn't know it would make things simpler to push to the I don't intend to tackle this more in-depth in this PR, but I have opened #24019 to track this. In the meantime, I've updated my commit to use the new image you've generated for this time -- thanks a lot! -- and CI is still passing. |
qmonnet
removed
the
dont-merge/blocked
|
@qmonnet do you think that we need to backport this at all? |
I've been wondering, I would say no. This PR was mostly to get back to a “normal” state from where we can easily upgrade Sphinx and dependencies next time. I see no particular reason to backport and generate new docs-builder images for the older branches, for which the build is working well (and for which we don't expect to regenerate the list of build dependencies in the future), plus I doubt the backports would apply cleanly. The only risk I see is if we try to backport other changes built on top of this PR in the future, like was done last time to 1.12, to some extent; but then we can still backport this PR if necessary when the time comes. There is a separate discussion regarding backports for Google Analytics, though, but that doesn't need the rest of this PR in my opinion. |
qmonnet
added
the
backport-pending/1.13
qmonnet
added
backport-pending/1.12
backport-done/1.12
aanm
added
needs-backport/1.13
qmonnet
added
backport-done/1.13
The objectives of this PR are:
make update-requirements) the other dependencies, as much as we can.Please refer to individual commits for details.
A new docs-builder image will be necessary, or the CI workflow will fail because of the old rstcheck not recognising the new syntax for its arguments.