Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Switching from Madoko to AsciiDoc for P4Runtime specification #510
Changes from 40 commits
4f8de37
adcfbe9
7659239
780d780
c43d938
3d45a90
207dd87
f6703ed
ef538d6
4b9875f
dbbfe09
faf7275
4c48e1b
4e277de
36c2602
8f0f754
7fbb83d
50b8bf5
9eae1e4
6a58e48
d56cb14
3d6caa4
c0e0239
25118c6
71bc292
c2355be
02f0f21
37a06b1
4360049
f19f1b6
8c06ca7
0ed60c4
ce784a1
ba93b11
a93198f
d9aea99
79ab44a
2fc304a
8b9c162
4182d31
5229d7c
5779ce4
8bc8f6b
09eda6a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to restore lines like these in the Makefile, otherwise running
make
in the docs/v1 directory fails to find the .svg and .png image files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, we have 2 use cases:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we can reduce all cases to behave in a way where whether it is a bare metal system, a VM, or a container, the Makefile has the commands that generate the .svg/.png files from the OpenOffice source files, by installing LibreOffice on the system, yes? I thought that was the preference that Chris expressed earlier, and that is why we only have the OpenOffice files checked in, not the .svg/.png files.
Even if there are cases where the .svg/.png files are sitting there, running
make
with a Makefile that contains rules for generating those files will only generate them if they are needed, so I don't see only advantages to having these rules in the Makefile that can generate them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one more quick comment, in case this is something I am confused about.
If there are any containers anywhere that anyone wants to use to generate the PDF & HTML for the P4Runtime spec, I think we should tell them: "Install LibreOffice in that container, or it won't work."
And if there is some container published by someone in the P4 community that you want to use, that installs asciidoctor, but not LibreOffice, I think we should ask them to update that container so that it does contain LibreOffice, even if LibreOffice is only needed for generating the P4Runtime spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree with you. Does this mean we need to implement it for the container running in the CI environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a very good idea to me that the container used by CI to build PDF and HTML should have LibreOffice installed, so that it can create the formats of image files required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dscano, can you please add back to the Makefile the rules for generating the .svg and .png files from the .odg files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I see you have added those rules back.
Can you please add
images
as a dependency of the generated PDF and HTML files, so that if the .svg and .png files do not currently exist on the file system, then runningmake
with no target will run the commands that generate them, before generating PDF and HTML files?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. So I added it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
svg are also generated by the current docs/v1/Makefile. Are they no longer necessary with AsciiDoc PDF and HTML?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, no. As you can see in the
.adoc
file, the figures used are PNGs."