Skip to content

Conversation

@dylannorthrup
Copy link
Contributor

@dylannorthrup dylannorthrup commented Oct 9, 2023

Motivation

I ran into the same problems as described in #7498 and looked into the issue. The underlying errors primarily stemmed from three repeated issues:

  • Having multiple instances of <meta name="description" ....> in the same page (which is against W3.org standards)
  • Having unclosed <meta> tags
  • Having an extra </div> tag

EDIT: Removed all module upgrades to reduce the scope of this PR.
In addition, the versions of modules in the current requirements.txt did not have the correct mime-type for .webp images and thus could not handle them properly. And, after updating the modules, running make epub encountered a ThemeError due to some included CSS and JS file paths having parameters.

Changes

This PR makes the following updates:

  • Alter _extensions/godot_descriptions.py to skip adding a new <meta name="description"...> tag if one already exists in context["metatags"]
  • Alter _extensions/godot_descriptions.py and _templates/layout.html to modify <meta ...> tags to <meta ... /> (i.e. having the meta element close itself)
  • Alter _templates/layout.html to remove the extra </div> element on line 69 (which was chosen over the </div> on line 75 because the indentation on the comments block appeared to indicate it should be inside the document block)
  • Update the css/custom.css and js/custom.js paths to remove appended parameters (which the comments indicated were intended to be used for cache busting after updates).
  • Update all versions in requirements.txt to match the versions in the pip.txt and docs.txt in the main branch of the RTD repo

Results

I have been able to generate a valid EPUB file that renders properly on my Mac in Books.app. I have not tested generation of any other documentation.

EDIT: Because the earlier version of the sphinx module does not have a valid mimetype for *.webp images, building an EPUB file will generate several of the following errors:

WARNING: unknown mimetype for _images/IMAGE_FILENAME.webp, ignoring

Those images will not be included in the resulting EPUB file, but I was unable to find a way to update the mime_suffixes list configured in the sphinx module.

I continue to get a WARNING: [Social card] image cannot be an SVG image, skipping... error when doing EPUB generation, but I was unable to identify where this was coming from and it does not appear to have caused any errors in the resulting EPUB file.

@mhilbrunner
Copy link
Member

mhilbrunner commented Oct 9, 2023

Thanks for contributing and looking into this! No time to test this yet, but the changes look good besides the one that I noted above at first glance. CI also fails on the requirements.txt.

@dylannorthrup
Copy link
Contributor Author

Thanks for contributing and looking into this! No time to test this yet, but the changes look good besides the one that I noted above at first glance.

No worries. I'll reply to the issue above inline.

CI also fails on the requirements.txt.

The newer version of sphinx, 7.2.6, requires python 3.9 (which is true for all sphinx releases after 7.1.2). I presume this is because the default python version for Ubuntu 20.04 (the version used for the build containers) is 3.8.4. I'm not entirely sure how to update the container version of python (and pip) to 3.9, but given python3.8 is EOSupport Oct 2024, this is an issue that'll need sorted eventually.

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

I had some more time to have another look and leave a few comments.

I agree upgrading our dependencies is worthwhile (and planned for some time), though doing it in this PR would mean this is likely going to need a lot of time for review and proper testing, seeing as it jumps multiple major versions for core things like Sphinx.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally by viewing the generated ePub in Okular 23.08.1, it works as expected.

@dylannorthrup
Copy link
Contributor Author

@mhilbrunner Please let me know if there's anything else needed for this PR. I should be much more responsive to requests going forward.

@mhilbrunner mhilbrunner merged commit 0a01bae into godotengine:master Nov 25, 2023
@mhilbrunner
Copy link
Member

mhilbrunner commented Nov 25, 2023

@dylannorthrup No, unless we notice something down the line - because I've just gone ahead and squashed and merged the PR :) Congrats, and thanks for keeping up with all the requests. Thanks for contributing!

@FrederickKDP
Copy link
Contributor

Hi, I have been trying to fix 3.x epub version #9485 . I tried to build locally and same problem. Could someone tell me if the same issues happened here?
Maybe I can try read how this developed and port the solutions back to 3.x

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants