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

Fix broken links in documentation #12

Merged
merged 3 commits into from
Mar 29, 2022

Conversation

samuel-dudik
Copy link
Contributor

@samuel-dudik samuel-dudik commented Mar 7, 2022

Update regarding STONEWORK-CONFIG.pdf:

  • Fixed broken internal links
  • Fixed overlap and overflow problems in some tables (.md file needs to be properly formatted)

pdflatex (default pdf engine) doesn't seem to work with anchor links.

Generated using:

pandoc STONEWORK-CONFIG.md -o STONEWORK-CONFIG.pdf --pdf-engine=weasyprint -c style.css

style.css:

body {
        font-size: 12px;
}

a {
        text-decoration: none;
}

table, th, td {
        border: 1px solid;
        border-collapse: collapse;
}

th, td {
        padding: 3px;
}

Signed-off-by: Samuel Dudík samuel.dudik@pantheon.tech

Signed-off-by: Samuel Dudík <samuel.dudik@pantheon.tech>
@ondrej-fabry
Copy link
Collaborator

I am not sure we need to commit the .pdf format of the STONEWORK-CONFIG.

Copy link
Collaborator

@fgschwan fgschwan left a comment

Choose a reason for hiding this comment

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

As Ondrej stated, it is probably not a good idea to put binary file into repository. Its hard to track changes between versions. People use it only for binaries that are vital for the project. This PDF document can be easily regenerated. Well at least by using the instruction mentioned above.

I would suggest to not commit the pdf file and instead make the above instructions how to get pdf version of the information in MD file as easy as possible for users. When i look into the makefile in root of the project, I see target generate-config-docs that does the MD and PDF generation. I would prefer extracting PDF to another makefile target and commiting only MD. If somebody needs a pdf, he can just run one make command to get it (without all the docker containers and all, it is only MD to PDF conversion).

PS: the existing pdf generation(https://github.com/PANTHEONtech/StoneWork/blob/main/scripts/gen-docs.sh#L75) and the one mentioned in PR are not the same, right? So if pdf generation needs to be updated to PR instructions then change the scripts/makefile/... .

@samuel-dudik
Copy link
Contributor Author

samuel-dudik commented Mar 15, 2022

The .pdf file was added because of README.md referencing it:

available as a single [PDF document][config-pdf]).

I removed it and I will update the reference, but I'm not sure where to put basic information about how to generate PDFs. Maybe in docs/README.md or docs/GENERATE-PDF.md.

I updated the Makefile with a universal pdf generation target.
Now you can generate a .pdf from any .md file using:

make target.pdf

make pdf will generate .pdfs for all .md files present in the repo. make pdf-clean will remove all .pdfs in the repo.

Everything is generated with the original pandoc snippet with minor fixes.
However, STONEWORK-CONFIG.md is the only file that contains internal links in the form of HTML tags. They are supported by Github markdown, but not by pandoc. Pandoc supports other methods, but they aren't compatible with Github markdown. The only compatible solution is to use implicit headers extracted from Markdown headlines, but this way the headlines have to always include full names.
So I chose to use the original snippet for all .md files except the STONEWORK-CONFIG documentation by editing the

pandoc /api/CONFIG.md -o /api/CONFIG.pdf \
line to use my updated pandoc snippet. The problem is that it requires the weasyprint package. The stonework-proto-rootgen Docker image is based on Ubuntu 18.04 which doesn't contain the weasyprint package and also uses an outdated pandoc version which is incompatible with weasyprint. The question is wheter to update the Dockerfile to a newer Ubuntu version (20.04) or download latest binaries for these packages directly and remain on the same Ubuntu version.

@fgschwan
Copy link
Collaborator

@samuel-dudik
I think you are making this more complicated than it needs to be. I was referring only to generation of one pdf file, the STONEWORK-CONFIG.pdf (if script gen-docs.sh is called from cnf then the name changes, but for stonework it creates only this one file). The generation of pdf files for all markdown files is not necessary. The md-to-pdf makefile target could give the impression that it is needed, but current implementation only converts Readme.MD. It is unclear to me for what purpose (it is not reference anywhere, just the makefile section comments hints that someone used it in releasing, maybe).

My idea was to extract the pdf creating from gen-docs.sh script so that calling this script (from Stonework or any other CNF) doesn't create pdf files. This should break the tendency to commit pdf files to repository (pdf not generated=pdf not commited to git repo). However, someone could have the need for pdf. For example release process could need this. So some way how to create the pdf is needed (for example makefile target).In documentation only thing needed is the replacement of reference to config pdf with info how to generate it (make docs/config/STONEWORK-CONFIG.pdf ?)

Your make %.pdf seems good enough for generating the Readme.pdf. Is it good enough for STONEWORK-CONFIG.pdf? There are some differences in pandoc arguments.
If you can't apply your way of pandoc call on STONEWORK-CONFIG.pdf then don't. If that script is called from other CNFs, then config.MD is generated the same way. That means the same probably will happen for other CNFs regarding your way of calling pandoc to create pdf. So probably old call of pandoc is needed.

Signed-off-by: Samuel Dudík <samuel.dudik@pantheon.tech>
Signed-off-by: Samuel Dudík <samuel.dudik@pantheon.tech>
@samuel-dudik
Copy link
Contributor Author

samuel-dudik commented Mar 25, 2022

I reverted all changes to PDF generation and added rule to .gitignore to ignore all .pdf files. I also added the problem regarding broken links discussed above to a separate issue #14.

@ondrej-fabry ondrej-fabry merged commit fe9e5f8 into PANTHEONtech:main Mar 29, 2022
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.

3 participants