Switching from Madoko to AsciiDoc for P4Runtime specification

[Dscano]

I am currently in the process of transitioning the P4Runtime specification from Madoko to AsciiDoc. Below is an overview of the remaining tasks:

• I need to fix the layout of two tables in the P4Runtime-spec.adoc file.
• Finalize the reorganization and changes necessary for folder structure to support the switch from Madoko to AsciiDoc.

[jafingerhut]

I tried make in the docs/v1 directory on a Linux system, and got this error:

$ make
make: *** No rule to make target 'P4Runtime-Spec.adoc', needed by 'P4Runtime-Spec.pdf'.  Stop.

Note the capitalization of the S in Spec. The file name in your PR is P4Runtime-spec.adoc with a lower-case s in spec.

I would recommend leaving the Makefile as it is now, and renaming the .adoc file to match the current capitalization of the Madoko source file.

[jafingerhut]

The file logo.png is missing.

[jafingerhut]

Need to update the markup for the URL at the end of Section 1 -- in the PDF output it shows up as https://...

[jafingerhut]

At least some cross-references to figures are rendered in PDF as "Figure Figure 1". In Madoko it was necessary to say "Figure [#crossref-tag]" in the source, but the "Figure" is redundant in AsciiDoc. Similarly for section and table references.

[jafingerhut]

In the first paragraph of Section 4, a sentence in the PDF ends with "are described in a laterChapter 5." It would be better if it were "are described in Chapter 5". Either that or if "later chapter" were a link to Chapter 5, like I see for some other cross-references.

[jafingerhut]

Most or all of the figures could use some tweaking on the percent of the column width they occupy, to make them smaller. See the P4 language spec AsciiDoc source for examples.

[jafingerhut]

You use [source,p4] at the beginning of snippet of Protobuf definitions. Does Rouge support [source,protobuf]? I am guessing from the existence of a lexer for Protobuf here that it should: https://github.com/rouge-ruby/rouge/blob/master/lib/rouge/lexers/protobuf.rb

[jafingerhut]

The paragraph at the end of Section 6.4.3 ActionProfile looks blank in the PDF, probably because in the source it has spaces at the beginning of each line. Something should be done to improve that, probably deleting those leading spaces so the text comes out as a normal paragraph after the bullet list.

It is not clear to me, but perhaps those spaces were there intending for that text to be part of the last bullet, or something like that? If so, there are probably different markups in AsciiDoc that can achieve that, but given that it looks like a separate paragraph in the current Madoko PDF output, making it look like a separate paragraph in the AsciiDoc PDF output is a good start.

Similarly for some text inside a bullet list in the section with title "Meter & DirectMeter".

Also for a bullet list in the section with title "Batch Atomicity".

I experimented in a few places with ways to achieve this in the P4 language spec AsciiDoc source. I don't recall exactly which parts right now, but if you have trouble finding examples of it, let me know and I can try to find them. In the P4-16-spec.adoc file, search for the string "interface of a Match-Action" for an example of using a line containing only + to continue a bullet item in the next paragraph (if I recall correctly), or surrounding a paragraph with -- lines before and after.

Here is some AsciiDoctor documentation on this topic: https://docs.asciidoctor.org/asciidoc/latest/lists/continuation/

[jafingerhut]

Compare the section heading "depths" of all section headings in the current Madoko PDF output, and the AsciiDoc PDF output. At least "Counter & DirectCounter" is Section 6.4.4 in Madoko PDF output, but Section 6.5 in AsciiDoc PDF output.

[jafingerhut]

In the section titled "Extern", need whitespace after "reserved range" and before the range.

[jafingerhut]

In one of the markups that should be "cite:", it is misspelled "cte:"

[jafingerhut]

In the section titled "Bytestrings" there are some LaTeX math formulas. There are a few such formulas in the P4 language spec, too, that I was able to get to render properly using AsciiDoc. Search for occurrences of "latexmath" in the P4 language spec AsciiDoc source for examples.

[jafingerhut]

There is at least one remaining occurrence of [@PSATranslation] that should be updated to cite:[PSATranslation]. Also one occurrence of [@PSAActionSelector].

[jafingerhut]

One cross-reference with text [P4 Entity Messages](#sec-p4-entity-msgs) section needs to be updated to AsciiDoc.

[jafingerhut]

There is an occurrence of "WriteRequests" that should be replaced with "WriteRequests" for AsciiDoc. See here for some examples of differences I found when translating the P4 language spec: https://github.com/jafingerhut/learn-asciidoc/blob/main/p4-spec-next/README-madoko-to-asciidoc-notes.md#monospace

[jafingerhut]

Probably most occurrences of using $ to mark up algebra or math should be replaced with a backtick, unless they need fancier symbols, then latexmath is probably needed. There are many occurrences of $ remaining in the P4Runtime AsciiDoc.

[jafingerhut]

The section heading for the section named "Examples of StreamError Messages" needs at least one blank line before it, or else AsciiDoc does not interpret it as a section header.

There is also an old Madoko-style cross-reference markup on the line before that.

Similarly for section heading "Changes in v1.3.0"

[jafingerhut]

In the section "Changes in v1.4.0", the first sub-bullet item under "Actions" needs to be nested one level deeper.

[jafingerhut]

In section "P4Runtime Entries files" the sequence of bash commands needs different markup. Not sure, but maybe [source,bash] will work?

[Dscano]

FYI, the comments were I "react" with a thumbs-up have been addressed in c43d938

[jafingerhut]

FYI, the comments were I "react" with a thumbs-up have been addressed in c43d938

Great! Please let me know when you are ready for another round of review, i.e. when for each review comment you have either made changes, or replied in some way that you believe no changes are needed.

[Dscano]

You use [source,p4] at the beginning of snippet of Protobuf definitions. Does Rouge support [source,protobuf]? I am guessing from the existence of a lexer for Protobuf here that it should: https://github.com/rouge-ruby/rouge/blob/master/lib/rouge/lexers/protobuf.rb

I applied it to a few proto code blocks (such as 6.1.2) so we can compare and see how it looks.

[jafingerhut]

svg are also generated by the current docs/v1/Makefile. Are they no longer necessary with AsciiDoc PDF and HTML?

[Dscano]

From my understanding, no. As you can see in the .adoc file, the figures used are PNGs."

[jafingerhut]

LGTM

[Dscano]

I removed the ls docs/v1/build command because we no longer generate the build directory as was previously done in the Makefile for the spec generation.

In my opinion, the ls docs/v1/build command acts as a canary to confirm that the spec generation is successful. Therefore, I believe it is important to reintroduce the generation of the build folder in the Makefile for the spec generation. What do you think about it?

[jafingerhut]

I removed the ls docs/v1/build command because we no longer generate the build directory as was previously done in the Makefile for the spec generation.

In my opinion, the ls docs/v1/build command acts as a canary to confirm that the spec generation is successful. Therefore, I believe it is important to reintroduce the generation of the build folder in the Makefile for the spec generation. What do you think about it?

It definitely seems like there should be some way to ensure that the CI run fails, if the desired output files (i.e. PDF and HTML) are not created.

I don't see how ls docs/v1/build could actually achieve that purpose, though.

If there were bash commands that checked for the existence of the .pdf and .html output files, and did an exit 1 if they did not exist, that sounds like it would do so.

But the ls docs/v1/build only fails if that entire directory does not exist, and the directory would be something you create as a separately almost-never-failing step before generating the .pdf and .html files.

[jafingerhut]

That madoko-lint step seems unlikely to ever be run, so all CI steps that are able to run are passing now.

[jafingerhut]

@Dscano @chrispsommers I am not sure what the next step is on this PR. Chris, is there some change you are expecting? Davide, any changes you are expecting to make before merge? I know you asked about a mechanism to detect CI doc build failures, but that seems like something that can be enhanced in follow-up PRs, if desired.

[Dscano]

@Dscano @chrispsommers I am not sure what the next step is on this PR. Chris, is there some change you are expecting? Davide, any changes you are expecting to make before merge? I know you asked about a mechanism to detect CI doc build failures, but that seems like something that can be enhanced in follow-up PRs, if desired.

I have nothing to add on my side

[chrispsommers]

That madoko-lint step seems unlikely to ever be run, so all CI steps that are able to run are passing now.

Can we remove that Madoko lint check job now that it's obsolete? Then the checks would be clean.

[jafingerhut]

That madoko-lint step seems unlikely to ever be run, so all CI steps that are able to run are passing now.

Can we remove that Madoko lint check job now that it's obsolete? Then the checks would be clean.

As far as I can tell, the PR already has changes to update that to a simple AsciiDoc lint check, which could be enhanced in the future to do more, but is a start: https://github.com/p4lang/p4runtime/pull/510/files#diff-02faf8edad299680def7e81a0a75dbd91a0b6be25950df67dda5df98a68c44d1

I don't know why Github isn't picking that up when running automated tests on this PR. Perhaps Github uses the current .github files when deciding which tests to run on a PR that modifies those files?

[Dscano]

That madoko-lint step seems unlikely to ever be run, so all CI steps that are able to run are passing now.

Can we remove that Madoko lint check job now that it's obsolete? Then the checks would be clean.

I have already removed it. From my understanding and research, nowhere in the repo madoko-lint is it mentioned.

[Dscano]

That madoko-lint step seems unlikely to ever be run, so all CI steps that are able to run are passing now.

Can we remove that Madoko lint check job now that it's obsolete? Then the checks would be clean.

As far as I can tell, the PR already has changes to update that to a simple AsciiDoc lint check, which could be enhanced in the future to do more, but is a start: https://github.com/p4lang/p4runtime/pull/510/files#diff-02faf8edad299680def7e81a0a75dbd91a0b6be25950df67dda5df98a68c44d1

I don't know why Github isn't picking that up when running automated tests on this PR. Perhaps Github uses the current .github files when deciding which tests to run on a PR that modifies those files?

Indeed, based on the GitHub workflow of the repo, the spec.yaml runs madoko-lint, even if it is no longer defined in the new spec.yaml.

[chrispsommers]

Merging, the Madoko-lint check seems spurious. Thanks @Dscano and @jafingerhut for the great work!