Conversation
|
Friendly ping @rsms |
rsms
left a comment
There was a problem hiding this comment.
Thank you for this. I agree that this is a cleaner solution than GF maintaining a fork :•)
Pipfile
Outdated
| fontbakery = "==0.10.4" | ||
| skia-pathops = "==0.8.*" | ||
| gftools = "==0.9.*" | ||
| gftools = "*" |
There was a problem hiding this comment.
Please pin the version to whatever GF requires.
Accepting "any version" can break CI (it has happened in the past)
misc/makezip2.sh
Outdated
| cp build/fonts/var/InterVariable-Italic.woff2 "$ZIPDIR/web/InterVariable-Italic.woff2" | ||
| cp misc/dist/inter.css "$ZIPDIR/web/" | ||
| cp misc/dist/inter.css "$ZIPDIR/web/" | ||
| cp build/fonts/googlefonts/Inter*.ttf "$ZIPDIR/googlefonts/" |
There was a problem hiding this comment.
(Please revert this change)
Was this accidental? This script (makezip2.sh) is used to create the offician distribution, so it shouldn't contain Google Fonts files.
If the intention was to get a zip file of the googlefonts make target, I suggest adding a custom zip ... invocation to the googlefonts make target.
There was a problem hiding this comment.
(Not affiliated with the Google Fonts team)
Why are there makezip.sh and makezip2.sh? Is the former obsolete?
There was a problem hiding this comment.
@mon-jai something like that, I forget. None of them are intended to be used directly, just to reduce the complexity of the makefile.
There was a problem hiding this comment.
Maybe we could delete the obsolete one?
There was a problem hiding this comment.
Thanks for the review.
This change is intentional. In order for the fonts to be included in Google Fonts, they must reside in a zip file that's attached to the release. The easiest way for me to do this is to bundle the gf edition fonts into the existing zip. If you want them in a separate zip, I can tweak the github action. Either way, whenever you cut a new version, we'll need the googlefonts fonts to be generated and included with the release.
There was a problem hiding this comment.
Got it! Thanks for clarifying.
Github actions is only used for CI. The actual release I do manually, so, we can add a second zip file e.g. "Inter-VERSION-GoogleFonts.zip" which can then be fetched from github releases (scripted or manually.)
I'll add a patch in a separate comment for you.
|
Patch for making a separate --- a/Makefile 2024-03-22 11:40:10.000000000 -0700
+++ b/Makefile 2024-03-22 11:43:07.000000000 -0700
@@ -424,6 +424,7 @@
# - step2 runs tests, then makes a zip archive and updates the website (docs/ dir.)
DIST_ZIP = build/release/Inter-${VERSION}.zip
+DIST_ZIP_GF = $(SRCDIR)/build/release/Inter-$(VERSION)-GoogleFonts.zip
dist:
@echo "——————————————————————————————————————————————————————————————————"
@@ -443,13 +444,15 @@
$(MAKE) -f $(MAKEFILE) -j$(nproc) clean
$(MAKE) -f $(MAKEFILE) -j$(nproc) all
$(MAKE) -f $(MAKEFILE) -j$(nproc) test
- $(MAKE) -f $(MAKEFILE) -j$(nproc) dist_zip dist_docs
+ $(MAKE) -f $(MAKEFILE) -j$(nproc) dist_zip dist_zip_gf dist_docs
$(MAKE) -f $(MAKEFILE) dist_postflight
dist_zip: | venv
- @#. $(VENV) ; python misc/tools/patch-version.py misc/dist/inter.css
bash misc/makezip2.sh -reveal-in-finder "$(DIST_ZIP)"
+dist_zip_gf: | venv
+ cd "$(FONTDIR)/googlefonts" && zip -q -X -r "$(DIST_ZIP_GF)" *.ttf
+
dist_docs:
$(MAKE) -C docs -j$(nproc) dist
@@ -468,7 +471,7 @@
@echo ""
@echo "——————————————————————————————————————————————————————————————————"
-.PHONY: dist dist_preflight dist_step1 dist_step2 dist_zip dist_docs dist_postflight
+.PHONY: dist dist_preflight dist_step1 dist_step2 dist_zip dist_zip_gf dist_docs dist_postflight
|
|
Apologies for the delay. Thank you so much for the patch. I agree that a separate zip is better. I've just implemented your feeback. I've pinned gftools, regenerated the lockfile and added your patch. |
|
I also don't mind deleting The team at GF still haven't decided whether to rename the family to "Inter 4" due to the amount of regressions it may cause. I should have an answer for you by Friday. Please don't merge the PR until then. |
|
@m4rc1e Thank you for the update. I'll hold off on merging until I hear back from you. |
|
Hey, So sorry about the delay. I went on a short break and Dave was off as well. The answer is in, we're not going to rename the family. I've also added |
|
Thank you for all your help @m4rc1e |
|
Thank you so much for the help and merge. Hate to be a bother but could you cut another release and include the gf zip? I can then onboard the family to Google Fonts. |
|
@m4rc1e unfortunately the change of gftools caused and implicit upgrade of fontTools which in turn has a bug (or is broken or I'm using it wrong; most likely) that messes up the wght values in the VF. I have almost no free time (between raising a newborn baby, running a start-up and sleeping a little bit in between) so I'm not sure when I'll get around to fixing the fontTools issues. We can try reverting the change to Pipfile but I assume you had a good reason for upgrading it. Alt we could try to pin fontTools to a version that doesn't break the VF STAT table. |
|
Thank you very much for the update. I have a nine month old who's teething so my sleep is completely wrecked,. I have no problems looking into this issue and opening another PR. I'll keep you posted. |
|
@m4rc1e I had a look this morning and I was wrong — the issue was introduced by an update to Glyphs app. I've got it working and will get you a zip in a moment (will post a link here) |
This pull requests adds a
googlefontstarget to the makefile. It will create a new set of variable fonts that comply with the GF spec. Personally, I feel this approach is cleaner than GF maintaining a fork.@rsms many people have requested an updated version of Inter on Google Fonts so I'm hoping this solution works for you,
google/fonts#3429. We may end up calling the family Inter 4 if we cannot all agree that the regressions are not going to cause people to complain.