Re: [PATCH v6 15/21] tests/qapi-schema: Add test of the rST QAPI doc-comment outputn

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 29 Sep 2020 at 13:20, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> In subject, s/outputn/output/
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > Add a test of the rST output from the QAPI doc-comment generator,
>> > similar to what we currently have that tests the Texinfo output.
>> >
>> > This is a bit more awkward with Sphinx, because the generated
>> > output is not 100% under our control the way the QAPI-to-Texinfo
>> > generator was. However, in practice Sphinx's plaintext output
>> > generation has been identical between at least Sphinx 1.6 and
>> > 3.0, so we use that. (The HTML output has had changes across
>> > versions). We use an exact-match comparison check, with the
>> > understanding that perhaps changes in a future Sphinx version
>> > might require us to implement something more clever to cope
>> > with variation in the output.
>> >
>> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> It's not just the potential Sphinx version dependence that makes this
>> awkward.
>>
>> We can no longer check what our doc generator does (at least not without
>> substantial additional coding), we can only check what it does together
>> with Sphinx.  We do so for one output format.
>>
>> Our doc generator output could change in ways that are not visible in
>> the Sphinx output format we test, but are visible in some other output
>> format.
>>
>> We choose to test plain text, because it has the lowest risk of unwanted
>> Sphinx version dependence, even though it probably has the highest risk
>> of "rendering stuff invisible".
>>
>> Certainly better than nothing, and probably the best we can do now, but
>> let's capture the tradeoff in the commit message.  Perhaps:
>>
>>   This is a bit more awkward with Sphinx, because the generated output
>>   is not 100% under our control the way the QAPI-to-Texinfo generator
>>   was. We can't observe the data we generate, only the Sphinx
>>   output. Two issues.
>>
>>   One, the output can vary with the Sphinx version.  In practice
>>   Sphinx's plaintext output generation has been identical between at
>>   least Sphinx 1.6 and 3.0, so we use that. (The HTML output has had
>>   changes across versions). We use an exact-match comparison check, with
>>   the understanding that perhaps changes in a future Sphinx version
>>   might require us to implement something more clever to cope with
>>   variation in the output.
>>
>>   Two, the test can only protect us from changes in the data we generate
>>   that are visible in plain text.
>>
>> What do you think?
>
> Yes, seems worth recording that in the commit message (especially
> now you've written the text :-)).

:)

>> > +# Test the document-comment document generation code by running a test schema
>> > +# file through Sphinx's plain-text builder and comparing the result against
>> > +# a golden reference. This is in theory susceptible to failures if Sphinx
>> > +# changes its output, but the text output has historically been very stable
>> > +# (no changes between Sphinx 1.6 and 3.0), so it is a better bet than
>> > +# texinfo or HTML generation, both of which have had changes. We might
>>
>> Texinfo
>>
>> > +# need to add more sophisticated logic here in future for some sort of
>> > +# fuzzy comparison if future Sphinx versions produce different text,
>> > +# but for now the simple comparison suffices.
>> > +qapi_doc_out = custom_target('QAPI rST doc',
>> > +                             output: ['doc-good.txt'],
>> > +                             input: files('doc-good.json', 'doc-good.rst'),
>>
>> Gawk at my Meson ignorance...
>>
>> Looks like this builds doc-good.txt from doc.good.json and doc-good.rst.
>>
>> doc-good.txt is also a source file.  Works, because we use a separate
>> build tree.  Might be confusing, though.
>
> Yes. We could change the name of the reference source file that
> we have checked into the git repo if you wanted. (The output file
> written by Sphinx has to be the same name as the input .rst file AFAICT.)

I'll see what I can do (and thanks for the hint).

>> > +                             build_by_default: build_docs,
>> > +                             depend_files: sphinx_extn_depends,
>> > +                             # We use -E to suppress Sphinx's caching, because
>> > +                             # we want it to always really run the QAPI doc
>> > +                             # generation code. It also means we don't
>> > +                             # clutter up the build dir with the cache.
>> > +                             command: [SPHINX_ARGS,
>> > +                                       '-b', 'text', '-E',
>> > +                                       '-c', meson.source_root() / 'docs',
>> > +                                       '-D', 'master_doc=doc-good',
>> > +                                       meson.current_source_dir(),
>> > +                                       meson.current_build_dir()])
>> > +
>> > +# Fix possible inconsistency in line endings in generated output and
>> > +# in the golden reference (which could otherwise cause test failures
>> > +# on Windows hosts). Unfortunately diff --strip-trailing-cr
>> > +# is GNU-diff only. The odd-looking perl is because we must avoid
>> > +# using an explicit '\' character in the command arguments to
>> > +# a custom_target(), as Meson will unhelpfully replace it with a '/'
>> > +# (https://github.com/mesonbuild/meson/issues/1564)
>>
>> Rather disappointing.
>>
>> > +qapi_doc_out_nocr = custom_target('QAPI rST doc newline-sanitized',
>> > +                                  output: ['doc-good.txt.nocr'],
>> > +                                  input: qapi_doc_out[0],
>> > +                                  build_by_default: build_docs,
>> > +                                  command: ['perl', '-pe', '$x = chr 13; s/$x$//', '@INPUT@'],
>> > +                                  capture: true)
>>
>> I figure this strips \r from the build tree's doc-good.txt.
>
> Close; it turns either CRLF or LF into the host OS's
> line-ending sequence (see below).
>
>> > +qapi_doc_ref_nocr = custom_target('QAPI rST doc reference newline-sanitized',
>> > +                                  output: ['doc-good.ref.nocr'],
>> > +                                  input: files('doc-good.txt'),
>> > +                                  build_by_default: build_docs,
>> > +                                  command: ['perl', '-pe', '$x = chr 13; s/$x$//', '@INPUT@'],
>> > +                                  capture: true)
>>
>> Uh, this strips it from the source tree's doc-good.txt, right?  Why is
>> that necessary?
>
> This is in case the user has a setup that eg has git
> doing line-ending conversion on checkout somehow. As a
> non-Windows user I opted to be belt-and-braces about
> converting both files to a known-consistent line ending.
> It's also necessary because the perl rune isn't really
> "delete \r"; it's "delete any \r and then output the
> line with the OS line ending" because the files it processes
> are being read and written in text mode. So the output
> will be \r\n on Windows and \n on Unix; the test passes
> in both cases because both files have the same
> line endings after conversion.

Uff.  Thanks!

Reviewed-by: Markus Armbruster <armbru@redhat.com>