Re: [PATCH 1/3 v3] docs: Add grub command documentation checks

[Hans Ulrich Niedermann]

On Fri, 24 Apr 2020 15:50:54 +0200
Hans Ulrich Niedermann <address@hidden> wrote:

> This adds the docs/check-commands.py script which checks whether
> grub commands implemented in the git source tree are documented
> in docs/grub.texi and vice versa.
> 
> During a standard "make" command, BUILT_SOURCES makes sure that
> docs/undocumented-commands.texi will be created (if it does not
> exist yet) or updated (if it differs from what the script would
> generate) or be left alone otherwise.
> 
> If you run "make grub.info" in the docs/ subdirectory, the
> BUILT_SOURCES trick to create or update undocumented-commands.texi
> will not work (this is an Automake limitation). So if you want
> to avoid the "all" make target, you need to explictly run
> "make update-undoc" first.
> 
> The generated docs/undocumented-commands.texi will document each
> otherwise undocumented command by describing that it is undocumented,
> and by adding that it is mentioned somewhere else in the info page if
> that happens to be the case.

The large RFC patchset for TXT boot just made me notice one potential
issue with this way of hooking that script into the build process:

Nobody will notice during patch reviews when a patch changes the set of
commands in the source code without changing the grub.texi
documentation accordingly.

The following are the technical details about why, and how we could
force missing documentation changes to show up during patch reviews.


What this patch does (requiring manual code/doc review)
=======================================================

As of this patch, the docs/check-commands.py script is called and
docs/undocumented-commands.texi is generated from scratch, and
there are no checks on the actual results:

  * Every "make" build will generate a new
    docs/undocumented-commands.texi file which will then be included
    from docs/grub.texi to be part of the generated grub info page.

    (This file should be in $(builddir), not in $(srcdir), but the
     older and current build rules for info pages only work in
     $(srcdir), so docs/undocumented-commands.texi is created in
     $(srcdir) to stay compatible with this inherited weirdness.)

  * If a patch adds or removes commands in the source code, a
    different docs/undocumented-commands.texi will be generated to
    reflect that.

    However, if nobody happens to notice that the patch changes the set
    of implemented commands, nobody will notice that documentation
    updates would be in order.

So patch reviews will not directly show that commands have been added
or removed from the codebase and that the documentation should perhaps
be adapted to reflect those changes.


What this could be changed to: Forced grub command doc review
=============================================================

If the grub project wants enforce that every single patch which
changes the set of implemented commands to also change the set of
commands documented in docs/grub.texi accordingly, an alternative
approach enforcing this would be the following:

  * Add docs/undocumented-commands.texi to the git repository.

  * Still have every "make" build update the
    docs/undocumented-commands.texi in the $(srcdir).

  * Now, if a patch (set) adds adds or removes grub commands in the
    source code, the generated docs/undocumented-commands.texi will be
    different from the docs/undocumented-command.texi from the git repo
    and thus this difference will show up in patches.

    This of course assumes that someone will at some time run "make"
    after applying the respective patch. I would expect a "make" run to
    be a part of all test processes, so I would expect this assumption
    to hold.

  * If the patch review process results in the decision that the patch
    should actually add undocumented grub commands to or remove
    documented grub commands from the source code, then the changes to
    docs/undocumented-commands.texi can be committed along with the
    change to the source code.

    However, if the patch review process results in the decision
    that the source code and the documentation should remain in sync,
    the further divergence will show up here, and a documentation update
    to docs/grub.texi to reflect the changes in the source code can be
    asked for in a revised version of the patch.

  * However, this forces changes to docs/grub.texi to be in the same
    patch as the changes to the grub source code.

    If you want it to be possible to have a patch set which contains
    first a patch with the code change and then another patch with the
    documentation update, this will result in a lot of ugliness with
    "make" changing git tracked files to different content. In that
    case, I would strongly advise to stay with the manual doc review
    process.

    Should there be any patches changing doc/check-commands.py which
    generates doc/undocumented-commands.texi, these generated changes
    must be contained the same patch for the same reason.


Conclusion
==========

Which of these two methods is preferable for grub is for other people
to decide. Both are valid models. I just needed to present that these
two alternative methods exist and can be chosen from.

> The build time requirements of the make target's build rule are
> already required by other parts of the grub buildsystem:
> 
>   * $(CMP)
>     compare two files by content
> 
>   * $(PYTHON)
>     check-commands.py has been written to run on Python 2 and 3
>     (tested with Python 2.7 and Python 3.7)
> 
> The docs/check-commands.py script runs fast enough to not perceivably
> slow down a "make" on the grub source tree. The script basically reads
> and more or less greps through docs/grub.texi and all *.c source
> files, and that happens very quickly.
> 
> Signed-off-by: Hans Ulrich Niedermann <address@hidden>
> ---
>  .gitignore             |   1 +
>  docs/Makefile.am       |  16 +++-
>  docs/check-commands.py | 205
> +++++++++++++++++++++++++++++++++++++++++ docs/grub.texi         |
> 4 + 4 files changed, 223 insertions(+), 3 deletions(-)
>  create mode 100644 docs/check-commands.py
> 
> diff --git a/.gitignore b/.gitignore
> index 678e829aa..ed019dd44 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -93,6 +93,7 @@ widthspec.bin
>  /docs/*.info-[0-9]*
>  /docs/stamp-1
>  /docs/stamp-vti
> +/docs/undocumented-commands.texi
>  /docs/version-dev.texi
>  /docs/version.texi
>  /ehci_test
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index 93eb39627..8e8500226 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -2,8 +2,18 @@ AUTOMAKE_OPTIONS = subdir-objects
>  
>  # AM_MAKEINFOFLAGS = --no-split --no-validate
>  info_TEXINFOS = grub.texi grub-dev.texi
> -grub_TEXINFOS = fdl.texi
> -
> -EXTRA_DIST = font_char_metrics.png font_char_metrics.txt
> +grub_TEXINFOS = fdl.texi undocumented-commands.texi
>  
> +EXTRA_DIST = check-commands.py font_char_metrics.png
> font_char_metrics.txt 
> +CLEANFILES = undocumented-commands.texi
> +BUILT_SOURCES = update-undoc
> +update-undoc:
> +     $(PYTHON) check-commands.py > undocumented-commands.texi.tmp
> +     @if test -f $(srcdir)/undocumented-commands.texi && $(CMP)
> undocumented-commands.texi.tmp $(srcdir)/undocumented-commands.texi;
> then \
> +             echo "Not updating undocumented-commands.texi: is up
> to date"; \
> +             rm -f undocumented-commands.texi.tmp; \
> +     else \
> +             echo "Updating undocumented-commands.texi"; \
> +             mv -f undocumented-commands.texi.tmp
> $(srcdir)/undocumented-commands.texi; \
> +     fi
> diff --git a/docs/check-commands.py b/docs/check-commands.py
> new file mode 100644
> index 000000000..4174b954a
> --- /dev/null
> +++ b/docs/check-commands.py
> @@ -0,0 +1,205 @@
> +#!/usr/bin/env python
> +#
> +# check-commands.py - compare commands defined in grub.texi the *.c
> sources +#
> +# Usage:
> +#     check-commands.py > undocumented-commands.texi.new
> +#
> +# There are no command line parameters. The locations of grub.texi
> and +# the *.c source files are determined relative to the location
> of the +# script file. The only output happens on stdout, in texinfo
> format +# for inclusione into grub.texi via '@include'.
> +
> +from __future__ import print_function
> +
> +import os
> +import re
> +
> +import sys
> +
> +
> +class CommandChecker:
> +
> +    def __init__(self):
> +        srcdir, self.prog = os.path.split(__file__)
> +        self.top_srcdir = os.path.dirname(os.path.abspath(srcdir))
> +
> +    def read_texi_text_commands(self, texi_filename):
> +        texi_pathname = os.path.join(self.top_srcdir, 'docs',
> texi_filename)
> +        command_re = re.compile(r'@command\{([a-zA-Z0-9_-]+)\}')
> +        commands = set()
> +        with open(texi_pathname) as texi:
> +            for line in texi.readlines():
> +                for m in command_re.finditer(line):
> +                    commands.add(m[1])
> +        return commands
> +
> +    def read_texi_command_menu(self, texi_filename):
> +        texi_pathname = os.path.join(self.top_srcdir, 'docs',
> texi_filename)
> +        menuentry_re = re.compile(r'\* ([^:]+)::')
> +        commands_node_re = re.compile(r'^@node .+ commands$')
> +        commands = set()
> +
> +        # Avoid using enum for the state machine state (for
> compatibility)
> +        waiting_for_node = True
> +        waiting_for_menu = False
> +        collecting_commands = False
> +
> +        with open(texi_pathname) as texi:
> +            for line in texi.readlines():
> +                line = line.strip()
> +                if line.startswith('@comment'):
> +                    continue
> +                if waiting_for_node:
> +                    if commands_node_re.match(line):
> +                        waiting_for_node = False
> +                        waiting_for_menu = True
> +                        continue
> +                if waiting_for_menu:
> +                    if line == '@menu':
> +                        waiting_for_menu = False
> +                        collecting_commands = True
> +                        continue
> +                if collecting_commands:
> +                    if line == '@end menu':
> +                        collecting_commands = False
> +                        waiting_for_node = True
> +                        continue
> +                    m = menuentry_re.match(line)
> +                    commands.add(m.group(1))
> +        return commands
> +
> +    def read_src_commands(self):
> +        top = os.path.join(self.top_srcdir, 'grub-core')
> +        register_re =
> re.compile(r'grub_register_(command|command_p1|extcmd)\s*\("([a-z0-9A-Z_\[]+)",')
> +        commands = set()
> +        for dirpath, dirnames, filenames in os.walk(top):
> +            for filename in filenames:
> +                filepath = os.path.join(dirpath, filename)
> +                filepath_ext = os.path.splitext(filepath)[1]
> +                if filepath_ext == '.c':
> +                    with open(filepath) as cfile:
> +                        for line in cfile.readlines():
> +                            for m in register_re.finditer(line):
> +                                commands.add(m.group(2))
> +        return commands
> +
> +
> +def write_undocumented_commands(undocumented_commands,
> +                                mentioned_commands):
> +    print("""\
> +@node Undocumented commands
> +@section The list of undocumented commands
> +""")
> +
> +    if not undocumented_commands:
> +        print("""
> +There appear to be no undocumented commands at this time.
> +""")
> +        return
> +
> +    print("""\
> +These commands are implemented in the grub software but still need
> to be +documented and sorted into categories.
> +""")
> +
> +    sorted_undocumented_commands =
> sorted(list(undocumented_commands)) +
> +    # Fit the longest command into the format string for the menu
> entries
> +    maxlen_str = sorted(list(undocumented_commands),
> +                        key=lambda cmd: len(cmd),
> +                        reverse=True)[0]
> +    fmt = '* %%-%ds %%s' % (2+len(maxlen_str))
> +
> +    print("@menu")
> +    for cmd in sorted_undocumented_commands:
> +        if cmd in mentioned_commands:
> +            print(fmt % ("%s::" % cmd, "Undocumented command (but
> mentioned somewhere)"))
> +        else:
> +            print(fmt % ("%s::" % cmd, "Undocumented command"))
> +    print("@end menu")
> +    print()
> +
> +    for cmd in sorted_undocumented_commands:
> +        print("@node %s" % cmd)
> +        print("@subsection %s" % cmd)
> +        print()
> +        print("The grub command @command{%s} has not been documented
> properly yet." % cmd)
> +        if cmd in mentioned_commands:
> +            print("""
> +However, the @command{%s} command @emph{is} mentioned
> +somewhere within this info file, so you might still find some
> +interesting information there.""" % cmd)
> +        print()
> +
> +
> +def print_set(st):
> +    for n, item in enumerate(sorted(list(st)), start=1):
> +        print("@comment", "  %d." % n, item)
> +
> +
> +def main():
> +    cc = CommandChecker()
> +
> +    print("""\
> +@comment Automatically generated by the %s script.
> +@comment Do not modify this generated file.
> +@comment
> +@comment If you want to document some of the commands listed below,
> feel +@comment free to copy over some of the @nodes and @subsections
> below +@comment into the grub.texi file proper and re-generate this
> file. +@comment""" % cc.prog)
> +
> +    # This gives a few good hints about what commands are at least
> +    # mentioned somewhere which can be helpful for finding out more
> +    # about commands.
> +    texi_text_commands = cc.read_texi_text_commands('grub.texi')
> +    print("@comment", "Commands mentioned somewhere in the grub.texi
> text:")
> +    print_set(texi_text_commands)
> +
> +    texi_node_commands = cc.read_texi_command_menu('grub.texi')
> +    # print("@comment", "Commands documented in grub.texi
> node/subsection:")
> +    # print_set(texi_node_commands)
> +
> +    src_commands = cc.read_src_commands()
> +    # print("@comment", "Commands registered in grub source:")
> +    # print_set(src_commands)
> +
> +    node_without_src = texi_node_commands - src_commands
> +    if node_without_src:
> +        print("@comment",
> +              "Cmds documented in grub.texi node/subsection but not
> registered in source:")
> +        print_set(node_without_src)
> +        print("@comment")
> +    else:
> +        print("""\
> +@comment There appear to be no commands documented in a grub.texi
> +@comment node/subsection but not registered in grub source.
> +@comment""")
> +
> +    src_without_node = src_commands - texi_node_commands
> +    if src_without_node:
> +        print("@comment",
> +              "Cmds registered in source but not documented in
> grub.texi node/subsection:")
> +        print_set(src_without_node)
> +    else:
> +        print("""\
> +@comment All commands registered in the source appear to have been
> +@comment documented in a grub.texi node/subsection. Congratulations!
> +@comment""")
> +
> +    print()
> +
> +    write_undocumented_commands(src_without_node, texi_text_commands)
> +
> +    # Once grub.texi actually documents all commands, we can
> uncomment
> +    # this and actually fail if the set of implemented commands and
> +    # the set of documented commands differ in any way.
> +    # if ((len(node_without_src) > 0) or (len(src_without_node) >
> 0)):
> +    #     sys.exit(1)
> +
> +    sys.exit(0)
> +
> +
> +if __name__ == '__main__':
> +    main()
> diff --git a/docs/grub.texi b/docs/grub.texi
> index d6408d242..0865ffa17 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3764,6 +3764,7 @@ shell}.
>  * General commands::
>  * Command-line and menu entry commands::
>  * Networking commands::
> +* Undocumented commands::
>  @end menu
>  
>  
> @@ -5636,6 +5637,9 @@ is given, use default list of servers.
>  @end deffn
>  
>  
> +@include undocumented-commands.texi
> +
> +
>  @node Internationalisation
>  @chapter Internationalisation
>