docs: auto-generated bnf files embed too much data

[knz]

So here's a situation I'm encountering on #21063 with an external contributor: https://teamcity.cockroachdb.com/viewLog.html?buildId=458831&buildTypeId=Cockroach_UnitTests

This is really a reflection on commit 9126d2f1fa33668024fbbd071867b40b24035743.

Notice how this contributor is now getting a linter error on generated doc files even though he was careful to exclude the new statement from docs.

There are two issues here:

• (short term) I still am missing information in the linter error output as to how to correct the error. Unfortunately neither the commit message on 9126d2f1fa33668024fbbd071867b40b24035743 nor the linter message help me (nor the external contributor) determine what to do to make the error go away.

  We really need some help to more effectively learn (and regularly be reminded) how to interact with the doc generator:

  • have the instructions about how to generate the docs in the error output
  • perhaps simply re-enerate the relevant bnf files preemptively every time the grammar is modified

• (long term) really we should not have to regenerate docs when modifying the grammar for statements that are to be excluded from docs!

  I notice that the bnf files introduced by commit 9126d2f1fa33668024fbbd071867b40b24035743 embed more information from the grammar than the SVG files did. The specific file that concerns me is "stmt_block.bnf". This embeds virtually the entire grammar and all the keyword lists -- including even things that ought to be excluded from docs. This means that now most changes to the grammar will require the developer to update this file.

  Is there no way to restrict the scope of each bnf file to just one thing? To exclude "non documented" features from them?

On the one hand, I agree that the SVG files were too brittle and too hard too re-generate exactly. However the change in commit 9126d2f1fa33668024fbbd071867b40b24035743 is causing too much data from the grammar to spill over to bnf files, in turn causing lint errors in places that really should not be causing them.

[knz]

The problem is worse than I thought. Just reordering the rules in the grammar file breaks things. See e.g. https://teamcity.cockroachdb.com/viewLog.html?tab=buildLog&buildTypeId=Cockroach_UnitTests_Check&buildId=459878

I think this is just not right. I worked hard to ensure sql.go wasn't checked in to the repo any more so that we don't have to deal with spurious diffs in generated files and merge conflicts. Now we have the same problem what the removal of sql.go was aiming to fix, just in a different area.

[knz]

Jordan just taught me what I need to do:

 make generate PKG=./docs/...

Also I'm noticing it's blazing fast. I do not see any reason not to do that only when needed - during doc generation, instead of committing the generated files.

[benesch]

I don't think those BNF files are ever used, actually. They exist solely to provide human-readable diffs of something approximating the railroad diagrams during review of SQL grammar PRs.

I'm with you, @knz: I hate checking in generated files. Showing diffs of generated files should be solved with better review tooling, not by checking them into source control. That said, checking them into source control solves a real problem and works today, and I don't see any easy way to teach Reviewable and GitHub to display the generated diffs. I think everyone who set this up (me, @dt, @mjibson) is very open to a better solution, though.

In the meantime we should streamline this process. At the very least, make generate should run on ./docs/... by default.

[knz]

How would you feel about a pre-commit hook that populates the diff into the commit message, so it shows up clearly during reviews?

[benesch]

I guess that would solve the conflicts-in-generated-files issue, but it would have rather adverse effects on the usefulness of git log.

[knz]

Perhaps we can "compress" the bnf changes to highlight the areas where the changes occur without copy-pasting the entire diff.

[maddyblue]

Closing because we now do this automatically during make.