-
Notifications
You must be signed in to change notification settings - Fork 90
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
base: main
Are you sure you want to change the base?
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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ jobs: | |
fetch-depth: 0 | ||
- name: Build spec | ||
run: | | ||
docker run -v `pwd`/docs/v1:/usr/src/p4-spec p4lang/p4rt-madoko:latest make | ||
docker run -v `pwd`/docs/v1:/usr/src/p4-spec p4lang/p4rt-asciidoc :latest make | ||
ls docs/v1/build | ||
- name: Upload spec to S3 | ||
uses: jakejarvis/[email protected] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ jobs: | |
fetch-depth: 0 | ||
- name: Build spec | ||
run: | | ||
docker run -v `pwd`/docs/v1:/usr/src/p4-spec p4lang/p4rt-madoko:latest make | ||
docker run -v `pwd`/docs/v1:/usr/src/p4-spec p4lang/p4rt-asciidoc :latest make | ||
ls docs/v1/build | ||
- name: Upload spec to S3 | ||
uses: jakejarvis/[email protected] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
FROM ruby:3.3.5 | ||
LABEL maintainer="P4 API Working Group <[email protected]>" | ||
LABEL description="Dockerfile used for building the asciidoc specification" | ||
|
||
RUN apt-get update && \ | ||
apt-get install -y cmake flex bison libglib2.0-dev libcairo2-dev libpango1.0-dev libxml2-dev libwebp-dev libzstd-dev libgdk-pixbuf-2.0-dev time | ||
|
||
RUN apt-get install -y libreoffice && \ | ||
gem install asciidoctor && \ | ||
echo 'gem: --no-document' > /etc/gemrc && \ | ||
gem install nokogiri && \ | ||
gem install rghost && \ | ||
gem install asciidoctor-diagram && \ | ||
gem install asciidoctor-plantuml && \ | ||
gem install asciidoctor-pdf --version 2.3.19 && \ | ||
gem install asciidoctor-pdf-cjk && \ | ||
gem install asciidoctor-lists --version 1.1.2 && \ | ||
gem install coderay pygments.rb thread_safe && \ | ||
gem install slim && \ | ||
gem install concurrent-ruby && \ | ||
gem install haml tilt && \ | ||
gem install asciidoctor-mathematical && \ | ||
gem install asciidoctor-bibtex &&\ | ||
git clone https://github.com/rouge-ruby/rouge &&\ | ||
cd rouge && \ | ||
git log -n 1 | cat && \ | ||
gem build rouge.gemspec && \ | ||
gem install rouge | ||
|
||
VOLUME ["/usr/src/p4-spec"] | ||
WORKDIR /usr/src/p4-spec |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
all: | ||
docker build -t p4rt-asciidoc -f Dockerfile.asciidoc . | ||
docker tag p4rt-asciidoc p4lang/p4rt-asciidoc:latest |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,32 @@ | ||
|
||
SPEC=P4Runtime-Spec | ||
|
||
all: html pdf build | ||
ROUGE_STYLE=github | ||
ROUGE_CSS=style | ||
|
||
all: ${SPEC}.pdf ${SPEC}.html | ||
|
||
build: | ||
mkdir -p build | ||
|
||
# Note: Brute-force image rendering; could use more precise file-by-file make rules | ||
images: build assets/*.odg | ||
soffice --convert-to svg --outdir build assets/*.odg > /dev/null 2>&1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem. So I added it |
||
soffice --convert-to png --outdir build assets/*.odg > /dev/null 2>&1 | ||
${SPEC}.pdf: ${SPEC}.adoc images | ||
time asciidoctor-pdf -v \ | ||
-a pdf-fontsdir=resources/fonts \ | ||
-r asciidoctor-mathematical \ | ||
-r asciidoctor-bibtex \ | ||
-r asciidoctor-lists \ | ||
-a rouge-style=$(ROUGE_STYLE) $< | ||
|
||
html: ${SPEC}.mdk p4.json assets/* images | ||
madoko -vv --png --odir=build ${SPEC}.mdk | ||
${SPEC}.html: ${SPEC}.adoc images | ||
time asciidoctor -v \ | ||
-r asciidoctor-mathematical \ | ||
-r asciidoctor-bibtex \ | ||
-r asciidoctor-lists \ | ||
-a rouge-css=$(ROUGE_CSS) $< | ||
|
||
pdf: ${SPEC}.mdk p4.json assets/* images | ||
madoko --pdf -vv --png --odir=build $< | ||
images: | ||
soffice --convert-to svg --outdir resources/figs resources/figs/*.odg > /dev/null 2>&1 | ||
soffice --convert-to png --outdir resources/figs resources/figs/*.odg > /dev/null 2>&1 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. From my understanding, no. As you can see in the |
||
build_spec_with_images: images all | ||
|
||
clean: | ||
${RM} -rf build | ||
/bin/rm -f ${SPEC}.pdf ${SPEC}.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.
I would not make a separate CI step. Doing this will build an ephemeral docker image, which will disappear by the time
build-spec
is run. Call themake -C docs/tools/
command just before the docker run command inbuild-spec
step.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 implemented the fix in this commit 5229d7c, but I'm now encountering a different issue.