Added embedding default fonts in the theme wheel #232
Conversation
glatosinski
force-pushed
the
glatosinski/embed-fonts-in-wheel
branch
3 times, most recently
from
086cb0b
to
c406541
Compare
* Added `sphinx_immaterial.resources` module that contains pre-downloaded fonts delivered with the wheel. * Updated `add_google_fonts` method so it first checks the `sphinx_immaterial.resources` module for bundled fonts, then it checks local cache, and in the end it downloads fonts from remote server * Added downloading default fonts during stage/wheel building to `sphinx_immaterial.resources` module * Added stub `sphinx_immaterial.resources` module Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com> Co-authored-by: Unai Martinez-Corral <umartinezcorral@antmicro.com>
| @@ -4,3 +4,4 @@ pydantic | |||
| typing-extensions | |||
| appdirs | |||
| requests | |||
| importlib-resources | |||
There was a problem hiding this comment.
It looks like there's a importlib.resources module that has been available as part of the std libs since python v3.7. Since python 3.6 has reached end-of-life and this theme requires v3.8+, does this mean we don't need a new external dependency? Is there some advantage to using the thrid-party importlib_resources lib?
There was a problem hiding this comment.
After some more thought, I don't think the cached default fonts needs to be an importable resource. We could just use
# from google_fonts.py or external_resource_cache.py
pathlib.PurePath(__file__).parent / f"{default_font_location}" / f"{default_font_name}"The importlib.resources module seems designed to let external API easily find a non-python asset from our pkg.
There was a problem hiding this comment.
Yes, we should not add an extra dependency --- we can just use importlib.resources that is built in. Basically importlib.resources is primarily useful to support cases where the package is stored not as plain files, e.g. in a zip file. In that case it just works transparently, while using __file__ will not. However, we already locate some files using __file__ so I think it is fine to just do that.
There was a problem hiding this comment.
importlib-resources is added due to the fact that Python versions below 3.9 do not have files in importlib.resources. But sure, I can do a workaround that will not use importlib-resources dependency
| from sphinx_immaterial.google_fonts import install_google_fonts | ||
|
|
||
| from importlib_resources import files | ||
| from sphinx_immaterial import resources | ||
|
|
||
| from sphinx_immaterial import DEFAULT_THEME_OPTIONS |
There was a problem hiding this comment.
This is causing a "chicken before the egg" problem because you are trying to import from the very pkg that this installs. The CI seems fine because the pkg deps are installed before the pkg is built. However, when I run pip install . in a fresh env, I get ModuleNotFoundError.
full traceback
Traceback (most recent call last):
File "path\to\repo\.venv-temp\Lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 353, in <module>
main()
File "path\to\repo\.venv-temp\Lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 335, in main
json_out['return_val'] = hook(**hook_input['kwargs'])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "path\to\repo\.venv-temp\Lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 118, in get_requires_for_build_wheel
return hook(config_settings)
^^^^^^^^^^^^^^^^^^^^^
File "path\to\temp\pip-build-env-415k6g8g\overlay\Lib\site-packages\setuptools\build_meta.py", line 338, in get_requires_for_build_wheel
return self._get_build_requires(config_settings, requirements=['wheel'])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "path\to\temp\pip-build-env-415k6g8g\overlay\Lib\site-packages\setuptools\build_meta.py", line 320, in _get_build_requires
self.run_setup()
File "path\to\temp\pip-build-env-415k6g8g\overlay\Lib\site-packages\setuptools\build_meta.py", line 485, in run_setup
self).run_setup(setup_script=setup_script)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "path\to\temp\pip-build-env-415k6g8g\overlay\Lib\site-packages\setuptools\build_meta.py", line 335, in run_setup
exec(code, locals())
File "<string>", line 33, in <module>
File "path\to\repo\sphinx-immaterial\sphinx_immaterial\__init__.py", line 6, in <module>
import docutils.nodes
ModuleNotFoundError: No module named 'docutils'
There was a problem hiding this comment.
To avoid this we could set a special environment variable that causes sphinx_immaterial/__init__.py not to import other stuff.
| install_google_fonts( | ||
| files(resources), | ||
| files(resources), | ||
| DEFAULT_THEME_OPTIONS["font"].values(), | ||
| ) |
There was a problem hiding this comment.
I don't think this will really solve the problem reported in #222. If a CI installs this theme, then the install process will error out if the CI runner doesn't have access to Google's font API (as similarly reported in #222).
I was under the impression that we'd have to include copies of the default fonts with the theme src to properly address #222. I do appreciate the attempt to update the default fonts at pkg install time, but it isn't really feasible (with respect to #222).
#222 would be better resolved by updating the default fonts' cache when npm run build is performed.
There was a problem hiding this comment.
When installing from the git repository directly, we have to download the npm packages (which requires internet access). However, the wheel package that we publish to PyPI includes the pre-built javascript and css bundles, and icons, and I think the idea here is that it could also include the Google fonts. So it would in fact solve the problem reported in #222 as long as the user does not wish to change the fonts from their default values.
There was a problem hiding this comment.
Yeah that was my thinking exactly.
I'm not sure if installing from git is preferred when the users' env has limited access to other domains.
There was a problem hiding this comment.
IMO, those are two different use cases:
- Installing the wheel is meant for users of the tool which want to consume a pre-built artifact.
- Installing from git|zip|tar is meant for developers and contributors which want to build the tool from sources.
Those two use cases are common in any "compiled|elaborated" software project. sphinx-immaterial does need "compilation|elaboration", despite the languages being both "interpreted". The elaboration is not only related to the fonts, but the set of dependencies (tools) is different from runtime deps.
There was a problem hiding this comment.
Installing from git/zip is also common for envs that don't have python's pip tool. For example, the python executable shipped with Ubuntu usually doesn't have pip or venv and the admin group (needed to use apt) isn't viable for the active user(s). Sure, there are alternative install methods (docker containers), but it usually falls on pkg maintainers when all else fails.
There was a problem hiding this comment.
I completely missed that comment, so I'm answering it in parts...
Where would that sdist zip be hosted?
Currently, we don't publish releases on github, but we could upload the archive as a release asset if we do. Maybe that is something that should be discussed as part of #223. However, installing from git does require npm and stuff.
There was a problem hiding this comment.
@2bndy5 any update to this feature? Is there an alternative implementation?
There was a problem hiding this comment.
Sorry, I got stumped on how to do this from setup.py without breaking installs in a fresh python venv. See this comment (and the following response) which talks about switching to fontsource instead of fetching fonts directly from the GoogleFonts API.
My only real problem with this PR (aside from unnecessary new dependency on importlib.resources -- see review discussion here) is not being able to install it in a fresh venv because it would require all theme deps be install-time deps as well. See this review discussion.
There was a problem hiding this comment.
If you or @glatosinski could satisfy the review concerns here, then I'd be happy to move this PR forward and implement the switch to fontsource CDN in a separate PR.
I'm currently enthralled with exploring my idea for #261.
There was a problem hiding this comment.
Ok, I'll look into it
This PR is related to #222, and proposes an implementation for one of the discussed variants:
Add the default Google fonts to this theme's package.It is a mechanism that we currently use to prevent downloading fonts every time we start a fresh build of documentation
Changes made in the PR:
sphinx_immaterial.resourcesmodule that contains pre-downloaded fonts delivered with the wheel.add_google_fontsmethod so it first checks thesphinx_immaterial.resourcesmodule for bundled fonts, then it checks local cache, and in the end it downloads fonts from remote serversphinx_immaterial.resourcesmodule to embed them in release wheelsphinx_immaterial.resourcesmodule