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 ReadTheDocs PDF build #64

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Conversation

SecondSkoll
Copy link
Contributor

Fixes issue outlined in #58

@SecondSkoll
Copy link
Contributor Author

SecondSkoll commented Jul 11, 2024

RTD is being a real pain. So, the issue was bound to RTD using the root of the git clone and then generating files in _readthedocs/<format>. Sadly this can't be changed - and while for HTML this doesn't have any negative effects - it does for the PDF generation which, due to how the title page is implemented, uses relative links to some files.

So this fix changes the relative links to reference the current directory, by pre-copying the relevant files into the directory used by RTD (because RTD for some reason isn't registering the existence of files included with latex_additional_files so the images and fonts just don't exist for the RTD PDF build). It also implements some changes to support local generation.

The next issue is that while this does work, it doesn't work on a PR as RTD does not build a PDF on a PR - and because PDF generation would automatically clean up that output folder it gets very upset at the fact we put files in there for them to use to generate the PDF. This is why there's a bunch of commits here - as I didn't discover this part until I submitted the PR... So, if you can't squash this I can submit a new, clean PR instead.

Weirdly, RTD offers no flexibility in how it handles cases where it does or doesn't generate PDFs, so there's a conditional cleanup in post_build.

All this because of what I assume is a bug in how RTD handles latex_additional_files (aka it doesn't handle them at all, and ignores them completely).

TL;DR: This is a hacky fix on top of a hacky fix all due to some issues in RTD. I'll see if I can get this fixed and circle back to this project and implement fixes properly - but it might take some time. It will all work for now.

I really thought RTD was just ignoring some files listed in latex_additional_files, but it turns out I need an updated prescription for my glasses... Well, this should all work which is the important thing. I've verified manual output and RTD build using a fork of the starter pack where I have RTD builds on. You can verify the built PDF by looking at the RTD versions in the bottom left menu here.

Copy link
Collaborator

@dviererbe dviererbe left a comment

Choose a reason for hiding this comment

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

LGTM, there are two nitpicks I would like to be addressed before merging.

note: I can not access https://canonical-test-starter-pack-pdfgen.readthedocs-hosted.com/en/rtd-subdir/ and the PR preview does not show a download as PDF link, but I trust you that it works now.

note: The link check errors are all expected and should not block the merge. They are fixed in #53 (not merged yet).

docs/Makefile Outdated Show resolved Hide resolved
.readthedocs.yaml Outdated Show resolved Hide resolved
Copy link
Contributor Author

@SecondSkoll SecondSkoll left a comment

Choose a reason for hiding this comment

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

You can try https://canonical-test-starter-pack-pdfgen.readthedocs-hosted.com/_/sharing/G3ZK1L59yDc9fFutCcA3kuZ5Lr7C8J1L?next=/en/rtd-subdir/ to check the build preview - I'm not sure what level of access the temporary sharing URL provides though.

.readthedocs.yaml Outdated Show resolved Hide resolved
Apply suggested changes

Co-authored-by: Dominik Viererbe <[email protected]>
Copy link
Collaborator

@dviererbe dviererbe left a comment

Choose a reason for hiding this comment

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

You can try https://canonical-test-starter-pack-pdfgen.readthedocs-hosted.com/_/sharing/G3ZK1L59yDc9fFutCcA3kuZ5Lr7C8J1L?next=/en/rtd-subdir/ to check the build preview - I'm not sure what level of access the temporary sharing URL provides though.

This works for me now and I could verify that the download PDF link works :)

Merging now, thanks again for fixing this!

@dviererbe dviererbe linked an issue Jul 11, 2024 that may be closed by this pull request
@dviererbe dviererbe changed the title FIx RTD build Fix ReadTheDocs PDF build Jul 11, 2024
@dviererbe dviererbe merged commit b655b04 into canonical:2.0-preview Jul 11, 2024
1 of 2 checks passed
@dviererbe dviererbe added the enhancement New feature or request label Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: PDF documentation broken
2 participants