users_guide: Convert mkUserGuidePart generation to a Sphinx extension [skip ci]
ClosedPublic

Authored by patrickdoc on Aug 11 2017, 9:30 PM.

Details

Summary

This removes all dependencies the users guide had on mkUserGuidePart. The
generation of the flag reference table and the various pieces of the man page
is now entirely contained within the Spinx extension flags.py. You can see
the man page generation on the orphan page https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/ghc.html

The extension works by collecting all of the meta-data attached to the
ghc-flag directives and then formatting and displaying it at flag-print
directives. There is a single printing directive that can be customized with
two options, what format to display (table, list, or block of flags) and an
optional category to limit the output to (verbosity, warnings, codegen, etc.).

New display formats can be added by creating a function generate_flag_xxx
(where xxx is a description of the format) which takes a list of flags and
a category and returns a new xxx. Then just add a reference in the dispatch
table handlers. That display can now be run by passing :type: xxx to
the flag-print directive.

flags.py contains two maps of settings that can be adjusted. The first is
a canonical list of flag categories, and the second sets default categories for
files.

The only functionality that Sphinx could not replace was the
what_glasgow_exts_does.gen.rst file. mkUserGuidePart actually just reads
the list of flags from compiler/main/DynFlags.hs which Sphinx cannot do. As
the flag is deprecated, I added the list as a static file which can be updated
manually.

Additionally, this patch updates every single documented flag with the data
from mkUserGuidePart to generate the reference table.

Fixes Trac #11654 and, incidentally, Trac #12155.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
wip/gen-flag-table
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 17146
Build 32285: arc lint + arc unit
patrickdoc created this revision.Aug 11 2017, 9:30 PM
patrickdoc updated this revision to Diff 13480.Aug 11 2017, 9:34 PM
  • Update gitignore
bgamari edited edge metadata.Aug 14 2017, 11:11 PM

This looks great! A few comments inline.

docs/users_guide/ffi-chap.rst
16

Thanks for filling these :since: annotations in!

docs/users_guide/flags.py
338

After fixing the registry issue described below, this inexplicably fails on my machine with,

Exception occurred:
  File "/opt/exp/ghc/ghc-landing/docs/users_guide/flags.py", line 337, in generate_output
    env.app.builder)
AttributeError: 'NoneType' object has no attribute 'builder'
392

This fails on my machine with,

Exception occurred:
  File "/opt/exp/ghc/ghc-landing/docs/users_guide/flags.py", line 391, in setup
    app.registry.domains['std'].directives['ghc-flag'] = Flag
AttributeError: 'Sphinx' object has no attribute 'registry'

It looks like it should instead be,

app.add_directive_to_domain('std', 'ghc-flag', Flag)
docs/users_guide/using.rst
1020

There are enough "irreversible" flags that perhaps we want to allow the :reverse: field to be omitted.

docs/users_guide/what_glasgow_exts_does.rst
34

While I love the fact that this allows us to build the users guide without building GHC, I am a bit weary of this falling out of date. I'll try to think about how we can fix this.

I've added some specific replies inline.

After some sleuthing (reading the "Created using Sphinx 1.4.9" at the bottom of the master docs online), I have determined that the issues you have come from different versions of Sphinx. Versions >1.6 (docs would indicate these are 1.6.2 and 1.6.3) implemented a module called registry that handles various names. If you were to upgrade, these issues should go away.

If you were inclined not to upgrade, I could definitely handle the add_directive issue. However, I am less sure about fixing the env.app problem.

docs/users_guide/flags.py
338

In 1.4.9 env.app has the comment "only set when update() is run." While in >1.6 it is set immediately.

392

Again, in 1.4.9 the entire registry module does not exist. This appears to have been added in 1.6.2.

As for using add_directive_to_domain(), it appears that the current version actually would work. That function checks that the given domain ('std') exists and checks that (because I passed a class and not an older format) I did not try to also provide other keyword arguements. It then runs the line I used.

I somewhat expected that Sphinx would not let me override an existing directive. The add_object_type() function creates a directive called ghc-flag as well, and I was replacing it with my own. But it does not appear to check if the directive already exists, so it would work in our case.

I'm fine with switching to the provided API, provided it works. However, I'm not sure which version would be more stable. The current version is perhaps overly direct, while the API version relies on an unchecked case that some may call a bug.

docs/users_guide/using.rst
1020

It in fact can be omitted. Only the :shortdesc: and :type: are mandatory.
When not provided, :reverse: defaults to the empty string, and :category: checks for a file default. If there is also no file default, an error is thrown to the effect of KeyError: <filename>.

The makeshift vim macro I whipped up to convert the old mkUserGuidePart data into this format was not smart enough to drop the :reverse: field when not needed.

docs/users_guide/what_glasgow_exts_does.rst
34

The mkUserGuidePart code was mostly just formatting this nicely: https://downloads.haskell.org/~ghc/latest/docs/html/libraries/ghc-8.2.1/src/DynFlags.html#glasgowExtsFlags. While not the best presentation, it should at least be accurate.

patrickdoc updated this revision to Diff 13533.Aug 16 2017, 3:52 PM
  • Fix compatibility for Sphinx versions 1.3.6 through 1.6.3

I updated the code to just use app, app.env, and app.builder. Then I built and checked with Sphinx 1.3.6, 1.4.9, and 1.6.3. I had to update the column width specifiers in flags.rst because the \Y{} shorthand was only introduced in 1.6.

If you were inclined not to upgrade, I could definitely handle the add_directive issue. However, I am less sure about fixing the env.app problem.

Indeed, I came to the same conclusion: it's not at all clear how to deal with env.app in a platform-specific manner.

While ideally we would support as many Sphinx versions as possible, in practice this has been a challenge as upstream isn't particularly careful about interface stability or migration paths for extensions. Perhaps instead we should simply give up and mandate Sphinx 1.6, checking for this in configure. I'm not entirely happy about doing this, but it seems like it may be necessary.

docs/users_guide/flags.py
392

I'm fine with switching to the provided API, provided it works. However, I'm not sure which version would be more stable. The current version is perhaps overly direct, while the API version relies on an unchecked case that some may call a bug.

It might be a good idea to bring this up with upstream, but keeping what you have here is fine with me

docs/users_guide/using.rst
1020

Ahh, I see. Perhaps it would be a easy to simply sed these empty :reverse: lines out of existence then?

docs/users_guide/what_glasgow_exts_does.rst
34

Right, I'm fine keeping your approach for the time being, but ultimately it would be nice if the documentation were derived from the implementation. Otherwise the two will inevitably fall out of sync. Perhaps we could factor out glasgowExtsFlags (and the necessary flag types) into a separate module and compile a small utility to generate this list (which could be compiled with the stage0 compiler).

For the time being though I think what you have here is fine (although we should probably add a note next to glasgowExtsFlags to ensure that contributors update the documentation when they change it.

Whoops, I realized I never submitted my review comments; sorry for the delayed review, @patrickdoc!

  • Clean up reverse flags
  • Add note to DynFlags.hs to keep the docs up to date

Indeed, I came to the same conclusion: it's not at all clear how to deal with env.app in a platform-specific manner.

Perhaps instead we should simply give up and mandate Sphinx 1.6

After a bit of refactoring, app is now the root of the references. Using app, app.builder, and app.builder.env fixed the compatibility problems. I successfully built using Sphinx 1.3.6, 1.4.9, and 1.6.3 yesterday, so I think we can support the older versions.

bgamari accepted this revision.Aug 18 2017, 9:34 AM
This revision is now accepted and ready to land.Aug 18 2017, 9:34 AM
This revision was automatically updated to reflect the committed changes.
thomie awarded a token.Aug 6 2018, 6:09 PM