-
Notifications
You must be signed in to change notification settings - Fork 33
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
Added embedding default fonts in the theme wheel #232
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
sphinx_immaterial[json,clang-format,keys,cpp] | ||
sphinxcontrib-details-directive | ||
sphinx-jinja | ||
pydantic | ||
importlib_resources |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,3 +4,4 @@ pydantic | |
typing-extensions | ||
appdirs | ||
requests | ||
importlib-resources | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,12 @@ | |
import setuptools.command.develop | ||
import setuptools.command.install | ||
import setuptools.command.sdist | ||
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 | ||
Comment on lines
+33
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 full traceback
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid this we could set a special environment variable that causes |
||
|
||
with open("requirements.txt", encoding="utf-8") as reqs: | ||
REQUIREMENTS = [reqs.readlines()] | ||
|
@@ -146,6 +152,11 @@ def run(self): | |
target = {"min": "build", "dev": "build:dev"} | ||
|
||
try: | ||
install_google_fonts( | ||
files(resources), | ||
files(resources), | ||
DEFAULT_THEME_OPTIONS["font"].values(), | ||
) | ||
Comment on lines
+155
to
+159
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, those are two different use cases:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I completely missed that comment, so I'm answering it in parts...
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @2bndy5 any update to this feature? Is there an alternative implementation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll look into it |
||
tgt = target[self.bundle_type] | ||
node_modules_path = os.path.join(root_dir, "node_modules") | ||
if self.skip_npm_reinstall and os.path.exists(node_modules_path): | ||
|
@@ -187,6 +198,8 @@ def run(self): | |
"*.html", | ||
"custom_admonitions.css", | ||
"theme.conf", | ||
"resources/*.response", | ||
"resources/*/*.response", | ||
], | ||
"sphinx_immaterial.apidoc.cpp.cppreference_data": ["*.xml"], | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
""" | ||
Module holding additional resources, i.e. built-in fonts | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-partyimportlib_resources
lib?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some more thought, I don't think the cached default fonts needs to be an importable resource. We could just use
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should not add an extra dependency --- we can just use
importlib.resources
that is built in. Basicallyimportlib.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
importlib-resources
is added due to the fact that Python versions below 3.9 do not havefiles
inimportlib.resources
. But sure, I can do a workaround that will not useimportlib-resources
dependency