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 PNA specification #128

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

Conversation

Dscano
Copy link

@Dscano Dscano commented Nov 23, 2024

I am currently transitioning the PNA specification from Madoko to AsciiDoc. This is the first PR to initiate the work.

PNA.adoc Outdated Show resolved Hide resolved
PNA.adoc Outdated Show resolved Hide resolved
PNA.adoc Outdated Show resolved Hide resolved
@jafingerhut
Copy link
Collaborator

Please ping me when you believe this is ready to go. It looks like there are still work on tables to be done, and probably some other things.

@Dscano
Copy link
Author

Dscano commented Nov 26, 2024

I propose using NOTE to replace Begin/End TBD

@jafingerhut
Copy link
Collaborator

I propose using NOTE to replace Begin/End TBD

Sounds reasonable to me. Go for it, and we can take a look at the PDF results, but I'd be surprised if there is any problem with the idea.

Signed-off-by: Davide Scano <[email protected]>
@Dscano
Copy link
Author

Dscano commented Dec 1, 2024

@jafingerhut I have finished converting PNA specification from Madoko to AsciiDoc.

  • Figures 2 and 3 are not present in the PNA.mdk, so I don't know how to map them in PNA.adoc.
  • In my opinion, it's worth developing the HTM version as well.
  • It's worth developing the same automation mechanisms used in p4runtime and p4-spec.

@jafingerhut
Copy link
Collaborator

There are two sections in the table of contents named "Revision history". Ideally there should be only one, which should be the first appendix.

@jafingerhut
Copy link
Collaborator

The current Madoko PDF has appendices A, B, C, etc. As of the time of writing this comment, the AsciiDoc PDF has one appendix, A, with separate sub-sections for each of what were appendices A, B, C, ...

If it is straightforward to make the AsciiDoc appendices be more like the original Madoko ones, that would be nice. I believe it is possible, as I think we did this for the P4 language spec AsciiDoc.

PNA.adoc Outdated
P4~16~ externs (e.g., counters, meters, and registers).

PNA is designed to model the common features of a broad class of NIC
devices. By providing standard APIs and coding guidelines, the hope is
to enable developers to write programs that are portable across
multiple NIC devices that are conformant to the PNA[^PNAPortability].
multiple NIC devices that are conformant to the PNA[1].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Insert whitespace before [1].

Copy link
Collaborator

Choose a reason for hiding this comment

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

We might also want to use different text than [1], since there are also auto-generated bibliography references that look like [1], too. Maybe [Footnote 1], both where it is referenced from, and where the footnote is shown?

PNA.adoc Outdated
target specific `pna.p4` include file. They are expected to differ
from one PNA implementation to another[^PNAP4TargetSpecific].
from one PNA implementation to another[1].
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend [Footnote 2] here, to avoid duplication with the earlier [Footnote 1].

@jafingerhut
Copy link
Collaborator

Thanks for updating the section headings in the appendices.

I would recommend removing the word "Appendix" from the AsciiDoc source for the titles of those appendices, because as of commit 5, the generated PDF output has titles like these for the appendices:

Appendix A: Appendix: Revision History

I believe the Appendix A is auto-generated, and looks fine to me, but the Appendix: part is from the AsciiDoc source, because Madoko did not auto-generate the word "Appendix" in the title.

Signed-off-by: Dscano <[email protected]>
build/${SPEC}.pdf: ${SPEC}.mdk
madoko --pdf -vv --png --odir=build $<

build/${SPEC}.pdf: p4.json
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 p4.json file should be removed with this PR, unless you find some reference to that file in the files that remain. I think p4.json was only used for Madoko syntax highlighting.

Makefile Outdated

build/${SPEC}.pdf: p4.json
build/${SPEC}.pdf: pna.p4
${SPEC}.pdf: ${SPEC}.adoc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add the file pna.p4 as a dependency on this rule, and the rule for generating HTML output, since PNA.adoc has include directives that include portions of pna.p4. If pna.p4 changes, it is likely that the PDF and/or HTML files need to be updated, too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, please add a Makefile rule to generate HTML output. I don't see one here. It would be helpful to have that, even if the original Makefile did not have such a rule to generate HTML from Madoko source.

Dscano added 2 commits January 4, 2025 09:25
Signed-off-by: Dscano <[email protected]>
Copy link
Collaborator

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

LGTM

@jafingerhut
Copy link
Collaborator

I will wait to merge this after mentioning this change at the next arch work group meeting on 2024-Jan-13.

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.

2 participants