-
Notifications
You must be signed in to change notification settings - Fork 450
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
WebP images in a gallery cause an unhandled exception during build #3671
Comments
Thanks for this very detailed report! I think we should do both of these:
As for this:
I think the best course of action would be to have a helper function that (a) ensures we have registered Would you be willing to send a pull request with a patch to resolve this? (It can focus only on webp for now). |
Yes, I already tried out some fixes very much like your recommendations (they work!) and plan to figure out how your tests work next, see how I should test the changes. I'm also accumulating a small branch of superficial fixes, such as quashing deprecation warnings I see during the test run. I'll submit it as a separate PR. #3673 |
Observation: If I modify the gallery RSS generation code to return a valid but empty RSS file, then no tests fail. So I conclude this isn't actually tested (Ah, and I belatedly realize the coverage output after the tests confirms this), and my intent right now is to create a minimal new test for the gallery RSS generator. Looking at a static site generator after so many years of looking at web API code for work is breaking my brain. I keep spending 30 seconds looking for the URL routing pointing to request handlers, before realizing, oh, hang on, that all doesn't exist here. :-) |
Observation: Adding a .webp image to nikola/data/samplesite/galleries/demo makes many tests fail (e.g. some with the build exception described above, but others because sitemap.xml is not generated, etc). Adding the line Imma submit it, tell me what you think... |
WebP images in a gallery cause `nikola build` to fail with an unhandled AttributeError exception, as described in getnikola#3671. This happens because the MIME type of WebP images is not known, and the resulting None value causes the RSS generation code to choke. This PR fixes that, by augmenting Python's list of known mimetypes with a hardcoded WebP one, before we ask for the mimetype of each image in a gallery. This addresses the above issue's checklist item "*Inject a hardcoded 'image/web' mimetype at runtime, before .guess_type() gets called.*", but does not complete the issue.
Required me to switch themes, galleries don't work in the previous one. Also, webp images don't work in galleries generally, but I'm using a patched version of Nikola to generate the site. Discussed in [#3671](getnikola/nikola#3671).
WebP images in a gallery cause `nikola build` to fail with an unhandled AttributeError exception, as described in getnikola#3671. This happens because the MIME type of WebP images is not known, and the resulting None value causes the RSS generation code to choke. This PR fixes that, by augmenting Python's list of known mimetypes with a hardcoded WebP one, before we ask for the mimetype of each image in a gallery. This addresses the above issue's checklist item "*Inject a hardcoded 'image/web' mimetype at runtime, before .guess_type() gets called.*", but does not complete the issue.
WebP images in a gallery cause `nikola build` to fail with an unhandled AttributeError exception, as described in getnikola#3671. This happens because the MIME type of WebP images is not known, and the resulting None value causes the RSS generation code to choke. This PR fixes that, by augmenting Python's list of known mimetypes with a hardcoded WebP one, before we ask for the mimetype of each image in a gallery. This addresses the above issue's checklist item "*Inject a hardcoded 'image/web' mimetype at runtime, before .guess_type() gets called.*", but does not complete the issue.
I don't want to sully your project with unclosed issues, so although there are a few checkbox items still left undone on this, I'm going to close it, because I don't think I'm going to get around to doing the last few items in the forseeable. Thanks for your patience and guidance getting the things done I wanted to see fixed! Thanks for Nikola! |
Environment
Python Version:: Python3.10.6 (and 3.11.0rc1 behaves the same)
Nikola Version: v8.2.3 (and latest git clone of commit 483ffd8 Wed Mar 1 22:52:02 2023 behaves the same)
Operating System: Linux Pop_OS 22.04 LTS (derived from Ubuntu 22.04)
Description:
Hi, good people! I am happy to look into contributing a PR to fix this myself, but wanted to discuss it first before wading in. Thanks!
Symptom
Adding a WebP image (with '.webp' extension) to my gallery causes
nikola build
to fail partway through, with a traceback (below). This happens even on a fresh site, created usingnikola init -dq site
.After the failure, running
nikola serve
displays the gallery correctly, including all the WebP images, but the gallery RSS feed is empty. Because this is so close to working, I'm hopeful that a small fix might be possible.I notice that galleries with an empty file named "image.webp" provoke this error, while gallery files named "image", with no extension, or "image.nonsense", are correctly ignored. So I infer that webp images are expected to work in galleries. (and the section below, "Any other image types we should handle?", seems to corroborate.)
The expected behavior is:
The Traceback
The problem stems from slightly earlier, nikola/plugins/task/galleries.py, line 753:
Where 'img' is the filename of the WebP image, ending with extension '.webp'.
.guess_type()
doesn't recognize the MIME type of this, and returns (None, None). The first of those None values causes problems a little later in the code (see the traceback), where it is used in XML/RSS generation.Analysis
The docs for the standard library function
.guess_type()
say that it recognizes the MIME types that are registered with IANA. They list 'image/webp' in a draft RFC, so this isn't yet an official MIME type.Sure enough, 'webp' is not included in the files consulted by the Python mimetypes package, which can be listed using:
So it's not surprising that guess_type() returns None.
Possible Solutions
I know nothing, so these might be bad ideas. Each checklist item is described below.
mimetype.guess_type()
.mimetypes.init()
(Rejected)Inject a hardcoded 'image/web' mimetype at runtime, before .guess_type() gets called
It sounds like the mimetypes module has ways to inject custom mimetypes into its internal list. We could add ".webp" -> "image/webp" before
.guess_type()
gets called.Add a test of gallery RSS content
A new test of RSS content would be useful, currently I think this is only verified in the CI baseline tests. Once such a test exists in the Python test suite, it (or a minor variation on it) could be used to verify the proposed fallback for unknown mimetypes (see the next item below).
Be robust to files with unrecognized mime type
Regardless of whether we decide to support WebP images here or not, we should be robust to
.guess_type()
returning (None, None) for files with unrecognized mimetype. There must be a more graceful way to handle this than ending the build with an unhandled exception. But what should that be?Check other image types we should handle.
Are there other images types that might work in a gallery but are impacted by this problem? If I'm going to look at this, I might as well systematically test them.
Although that did uncover some other problems, tracked there, none of the other image types break the build like this. They all have a recognized mimetype.
Review other calls to
mimetype.guess_type()
The code contains three calls to
mimetype.guess_type()
:Do calls (2) and (3) also need guarding against (None, None) return values?
Review whether "producing an RSS enclosure" is duplicated code.
Do calls (1) and (2) above represent duplicated code, to produce an RSS enclosure, that should be extracted and shared in a function somehow? On first glance I don't think so, but plan to review the idea and would be interested to hear any opinions.
Rejected ideas
Rejected idea: Call
mimetypes.init()
My first glance at the mimetypes docs pages made me think we ought to be calling
mimetypes.init()
before calling.guess_type()
. But later I noticed the docs says:So we don't need to call
.init()
explicitly.Rejected idea: Consider manually adding the webp mime type to my machine
I see StackOverflow questions about not-entirely-dissimilar problems in other software, which are also caused by the lack of a known MIME type for WebP images. A suggested solution there is to add the 'image/webp' MIME type to one of the 'knownfiles' listed above.
This sounds like a temporary workaround for users who hit this today. But editing these files presumably conflicts with the distributions management of them, which will have bad side effects (e.g. updates could overwrite the user's changes.)
Rejected idea: Consider automatically adding the webp mime type on Nikola install
Instead of suggesting to users that they change the contents of the 'knownfiles' listed above, we could do it automatically for them on Nikola install. This sounds like a bad idea which could cause problems for us in the future (e.g. when our changes are overwritten by an automated update of the changed file, leaving the user in a broken state again.)
Also, this would probably require admin/root privs, which most Nikola installs (hopefully) do not have.
Thanks! ❤️
Huge thanks for all the amazing Nikola goodness! I've been using and loving it for years!
The text was updated successfully, but these errors were encountered: