Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs: fix warnings for documentation build, use a linter #16407

Merged
merged 7 commits into from Jun 9, 2021
Merged

docs: fix warnings for documentation build, use a linter #16407

merged 7 commits into from Jun 9, 2021

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jun 2, 2021 •
edited

Four big steps in the PR:

  1. Fix the checks for warnings on builds for the documentation (1st commit).
  2. Address build warnings (2nd commit).
  3. Introduce a linter, rstcheck, for the RST documentation files (3rd commit).
  4. Fix issues reported by linters in the documentation (remaining commits).

Please refer to individual commit logs for details.

@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Jun 2, 2021
@qmonnet qmonnet requested a review from a team as a code owner June 2, 2021 16:56
@qmonnet qmonnet requested a review from joestringer June 2, 2021 16:56
@joestringer
Copy link
Member

Why aren't the doc tests picking up on these in the PR that introduced the lines? Docs warnings are intended to break the build so we can give the feedback to the original developer so they will learn and get these things right before we merge, rather than relying on folks like you to follow up.

@qmonnet qmonnet marked this pull request as draft June 4, 2021 01:21
@qmonnet
Copy link
Member Author

qmonnet commented Jun 4, 2021

Ok so there are several things here.

  • The :caption: were not reported by Sphinx at all. I noticed them by running a linter on the RST files. I should have mentioned it but I wrote this commit some time ago, and forgot it when I wrote the commit log.
  • In fact, the :caption: for .. code-block seems to be correct - It looks like a false positive from the tool, which only integrates so well with Sphinx :/.
  • After some investigation, it happens that the .. code-block:: txt are not blocking the build because we check for warnings on sphinx-build -b spelling, but not on sphinx-build -M html (which eventually build the HTML documentation and produces these warnings).

I marked as draft and will rework this PR:

  • I'll remove the change for the :caption:.
  • I'll update Documentation/check-build.sh to account for warnings at build time.
  • I found another linter which seems to work better with Sphinx, I'll see if there's an easy way to integrate it and to fix the issues it may report.

@qmonnet qmonnet changed the title docs: fix warnings for documentation build docs: fix warnings for documentation build, use a linter Jun 4, 2021
@qmonnet qmonnet marked this pull request as ready for review June 4, 2021 14:09
@qmonnet qmonnet requested a review from a team as a code owner June 4, 2021 14:09
@qmonnet qmonnet requested a review from joestringer June 4, 2021 14:09
@qmonnet
Copy link
Member Author

qmonnet commented Jun 4, 2021 •
edited

[Apparently, adding rstcheck to requirements.txt is not enough to be able to call the binary :/]

[EDIT] Hmm or do we need the PR to be merged first to have docs-builder updated with the new requirements? 🤔

joestringer
joestringer approved these changes Jun 7, 2021
View reviewed changes
Documentation/check-build.sh
@@ -85,4 +85,11 @@ fi
# fi

echo "Building documentation (${target})..."
exec sphinx-build -M "${target}" "${script_dir}" "${build_dir}" $@
sphinx-build -M "${target}" "${script_dir}" "${build_dir}" $@ -q 2> >(tee "${warnings}" >&2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, what's the difference between having the exec at the start vs. not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked the author of the code (Ilya) about that before doing the change, and he answered that it was mostly a “stylistic thing”, that “when a script calls one command at the end and that command is relatively heavyweight, [he] tends to use exec to sort of signify the purpose of the script”.

But I had to remove it because exec-ing into the command substitutes that command to the shell, and we wouldn't return to run the new instructions that my commit adds below.

@qmonnet qmonnet added the dont-merge/blocked Another PR must be merged before this one. label Jun 9, 2021
@qmonnet
Copy link
Member Author

qmonnet commented Jun 9, 2021

#16474 (better) addresses some .. code-block:: items and should probably go first.

@qmonnet
docs: report warnings when building documentation
b5e9fe8 
The check-build.sh script would check for warnings produced by running
"sphinx-build -b spelling", but not those produced when generating the
documentation with "sphinx-build -M html".

Let's reuse the warnings.txt file to store these warnings, if any, and
to return with a non-0 exit code in that case.

Notes: The file can be safely truncated (we won't reach that step if
warnings were generated from "sphinx-build -b spelling"). Also we do not
have to care of errors produced by "sphinx-build -M html", as they would
translate in an error exit code that would make the script return before
we look at the warnings.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: fix warnings for documentation build
3efca8a 
"txt" is not a valid language to pass to "code-block" directives.
Passing "none" instead of "txt" would work; but there is no need to mark
the snippet as a "code-block" if no syntax highlighting is required,
simply using a literal block is enough.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: pass a linter on the RST files
3de2451 
Use rstcheck (https://github.com/myint/rstcheck/) as a linter for the
RST Documentation files.

Run the linter before syntax and spelling validation by Sphinx, because
it goes much faster, so we fail faster if an error happens at that
stage.

Ignore the following items:

- Custom directives: tabs (from Sphinx extensions).
- Custom roles: any role added with the extlinks extension.
- Language: skip linting for bash in ".. code-block:: bash" snippets.
- Messages:
  - Skip an error on ordinated lists in bpf.rst (we have list with
    decreasing numbers, and there is also a small indent issue in a bit
    enumerated list in the file, but Sphinx handles it well so let's
    leave it untouched).
  - Hyperlink target not referenced: rstcheck does not seem able to
    recognise when targets are referenced from a different file.
  - Duplicate implicit target name: this happens when HTML anchors
    generated from section titles collide with explicit targets (or
    other section titles on the same page). This is harmless and we are
    not looking to fix those.
  - Malformed tables: it would be useful to have these reports if
    rstcheck would support ignoring substitution ("|PATTERN|") in grid
    tables. I reported a bug on rstcheck's tracker.
- Substitutions: any custom substitution defined in conf.py.

Note the existence of an alternative tool, rst-lint
(https://github.com/twolfson/restructuredtext-lint), but from my
experiments it does not integrate well with Sphinx at the moment.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: remove duplicate explicit targets from RST files
1bc9438 
The syntax for inline hyperlinks can take one or two underscores at the
end of the marker. With one underscore, it is an explicit target, which
means that the text for the link can be reused elsewhere without having
to copy the URL again. With two underscores, the reference is anonymous
and cannot be reused elsewhere.

Avoid having multiple explicit targets in the documentation. Let's go
for the easiest possible fix: add an additional underscore to make the
targets anonymous.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: indent code blocks in enumerated lists
e02b926 
Following a report from rstcheck, indent the code blocks used in
enumerated list items, to make them part of these items instead of
"breaking" the list.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet added needs-backport/1.10 and removed dont-merge/blocked Another PR must be merged before this one. labels Jun 9, 2021
@qmonnet
Copy link
Member Author

qmonnet commented Jun 9, 2021

Rebased, now that #16474 has been merged.

@joestringer
Copy link
Member

I pulled this PR, build a new cilium/docs-builder image (tagged with 2021-06-09 and latest) and pushed it to docker.io. You should be able to re-run the docs GH action now and validate the changes in the PR.

@qmonnet
docs: fix a few JSON snippets
fcf080c 
A JSON snippet in alibabacloud-eni.rst is not valid JSON, there is a
spurious comma after the last element of an array. Let's remove it to
make the JSON valid.

Let's also update the declaration for two blocks that were recently
marked as JSON. The ouptut contained in these blocks is made of JSON
fragments, but each block contains _several_ fragments without a root
object or array, making the entire snippet invalid JSON and trigerring
reports from the linter. Let's simply use literal blocks instead.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: split a C snippet in BPF documentation
a10ebe2 
A snippet of code was recently marked as C, but it actually contains
both C and a sample output from the kernel verifier (which is not C
code). Sphinx raises a warning as it fails to apply syntax highlighting
for C for that snippet. Let's split the C code from the verifier output.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet requested a review from a team June 9, 2021 22:13
@joestringer joestringer merged commit 29bf120 into cilium:master Jun 9, 2021
@qmonnet qmonnet deleted the pr/doc_fixes branch June 10, 2021 09:45
@gandro gandro mentioned this pull request Jun 14, 2021
Merged
@aanm aanm mentioned this pull request Jun 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@qmonnet @joestringer @gandro @aanm