Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switching from Madoko to AsciiDoc for P4Runtime specification #510

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

Dscano
Copy link
Contributor

@Dscano Dscano commented Nov 16, 2024

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
Copy link
Contributor

jafingerhut commented Nov 16, 2024

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
Copy link
Contributor

jafingerhut commented Nov 17, 2024

The file logo.png is missing.

@jafingerhut
Copy link
Contributor

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

@jafingerhut
Copy link
Contributor

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
Copy link
Contributor

jafingerhut commented Nov 17, 2024

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
Copy link
Contributor

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
Copy link
Contributor

jafingerhut commented Nov 17, 2024

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
Copy link
Contributor

jafingerhut commented Nov 17, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@jafingerhut
Copy link
Contributor

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

@jafingerhut
Copy link
Contributor

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
Copy link
Contributor

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

@jafingerhut
Copy link
Contributor

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

@jafingerhut
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

jafingerhut commented Nov 17, 2024

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
Copy link
Contributor

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

@jafingerhut
Copy link
Contributor

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

@Dscano
Copy link
Contributor Author

Dscano commented Nov 17, 2024

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

@jafingerhut
Copy link
Contributor

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
Copy link
Contributor Author

Dscano commented Nov 19, 2024

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.

* **Expected behavior**: Counter entry at index 0 is read. Notice that the
`index` subfield is missing under the `index` field message of
`CounterEntry` in the text dump of the message. This is because the
subfield is a scalar numeric type and 0 is therefore its default
value. Scalar fields with default values are omitted from the textual
representation of Protobuf messages.

2. Reading all counter entries by leaving the `index` field unset

. Reading all counter entries by leaving the `index` field unset
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PDF shows a leading "1."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is a 1 instead of a 2, then there must be something between the first and second item that makes asciidoctor think they are two separate numbered lists. Perhaps the blank line between them should be removed?


P4~16~ supports 2 different classes of enumeration types: without underlying
type (safe enum) and with underlying type (serializable enum or "unsafe" enum)
[@P4Enums]. For `enum` types with no underlying type --- as well as `error` ---
cite:[P4Enums]. For `enum` types with no underlying type --- as well as `error` ---
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the [Madoko?] symbol for em-dash --- is not rendering the way we want

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this page on asciidoctor, it looks like an em-dash should be specified by using two dashes -- in the source file, not three.



## Digest Messages
=== Digest Messages

See the [DigestEntry](#sec-digestentry) section.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't creating a link (at least in the PDF)

@jafingerhut
Copy link
Contributor

https://github.com/jafingerhut/learn-asciidoc/tree/main/p4runtime-spec-next/sample-generates-files

The bullets don't look like that when I view the PDF in any of these ways:

  • in the Google Chrome browser on a macOS system, on github.com
  • local PDF file viewed in Preview app on a macOS system.
  • local PDF file viewed with the Evince Document Viewer started by the command xdg-open P4Runtime-spec.pdf on an Ubuntu 24.04 Linux system

How did you view the PDF file that makes the bullets look like that?

@chrispsommers
Copy link
Collaborator

General comment - it looks fantastic, even better than the previous Madoko specs IMO. The overall appearance is much more modern. The HTML ToC on the left sidebar is awesome. I've found a few nits here and there which shouldn't block this from approval, but it would be nice to preserve all pending feedback somewhere, perhaps in a new issue or some other reasonable method. Thanks to @Dscano and @jafingerhut for their significant efforts on this. I left some specific comments inline, below are some general ones.

Minor stuff:

  • The #, \and > symbols in "code" blocks are getting weird red-brown highlighting
  • Nested bullets sometimes use the same symbol for each level (PDF) - HTML looks like expected
  • HTML rendering of inline code is rather distracting - text is smaller with a rounded border and shading. I think a simple font/color change, like the PDF rendering, would be easier to read.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great effort!

@chrispsommers
Copy link
Collaborator

https://github.com/jafingerhut/learn-asciidoc/tree/main/p4runtime-spec-next/sample-generates-files

The bullets don't look like that when I view the PDF in any of these ways:

  • in the Google Chrome browser on a macOS system, on github.com
  • local PDF file viewed in Preview app on a macOS system.
  • local PDF file viewed with the Evince Document Viewer started by the command xdg-open P4Runtime-spec.pdf on an Ubuntu 24.04 Linux system

How did you view the PDF file that makes the bullets look like that?

Downloaded the PDF into a Windows machine and viewed with Chrome:
image

@jafingerhut
Copy link
Contributor

https://github.com/jafingerhut/learn-asciidoc/tree/main/p4runtime-spec-next/sample-generates-files

The bullets don't look like that when I view the PDF in any of these ways:

  • in the Google Chrome browser on a macOS system, on github.com
  • local PDF file viewed in Preview app on a macOS system.
  • local PDF file viewed with the Evince Document Viewer started by the command xdg-open P4Runtime-spec.pdf on an Ubuntu 24.04 Linux system

How did you view the PDF file that makes the bullets look like that?

Downloaded the PDF into a Windows machine and viewed with Chrome: image

Sorry for crossing conversations here -- I was replying to Steffen's comment showing a screen shot where the top level bullets seemed to be much further left than what you showed.

I agree it is not ideal that all levels of bullets use the same symbol as the bullet. This was a workaround for an earlier problem we encountered where if we tried to use different symbols, the font we were using did not have the appropriate level 2 and level 3 nested bullet characters in the font's table, and it used empty rectangles for them, which looks worse than using the same character for all levels of nesting (in my opinion).

Achieving different symbols for different levels of bullet nesting would require either using a different font with those symbols defined properly, or editing the font file we have to define those symbols with reasonable shapes.

@Dscano
Copy link
Contributor Author

Dscano commented Dec 18, 2024

@chrispsommers @smolkaj @jafingerhut Thank you for the comments. I will address them as soon as possible.

@jafingerhut
Copy link
Contributor

@chrispsommers @smolkaj @jafingerhut Thank you for the comments. I will address them as soon as possible.

And if you are willing to merge this in before the list of things you have pointed out have been changed, Davide and I can use the comments on this PR (even after it is closed, linked from a new open issue), to track the comments you have made on this PR.

@chrispsommers
Copy link
Collaborator

@chrispsommers @smolkaj @jafingerhut Thank you for the comments. I will address them as soon as possible.

And if you are willing to merge this in before the list of things you have pointed out have been changed, Davide and I can use the comments on this PR (even after it is closed, linked from a new open issue), to track the comments you have made on this PR.

@smolkaj I'm OK with merging now and following up with the minor issues as time allows; wdyt?

@jafingerhut
Copy link
Contributor

I believe I have captured all of the net-yet-resolved comments on this PR in their own separate issues on this repo. All of them have had the new label "asciidoc-issue" added to them.

@chrispsommers
Copy link
Collaborator

I believe I have captured all of the net-yet-resolved comments on this PR in their own separate issues on this repo. All of them have had the new label "asciidoc-issue" added to them.

Thanks, I saw that. Waiting for @smolkaj 's opinion, I'm ready to merge if he is.

@@ -11,19 +11,19 @@ on:
- '*-dev'

jobs:
madoko-lint:
asciidoc -lint:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the space between asciidoc and -lint is causing this failure: https://github.com/p4lang/p4runtime/actions/runs/12320523043

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check failures need to be addressed

@Dscano
Copy link
Contributor Author

Dscano commented Dec 24, 2024

I had to update the Dockerfile.asciidoc because the previous version was not working correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants