Skip to content
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

make: add googlefonts target #678

Merged
merged 4 commits into from
May 15, 2024
Merged

make: add googlefonts target #678

merged 4 commits into from
May 15, 2024

Conversation

m4rc1e
Copy link
Contributor

@m4rc1e m4rc1e commented Feb 6, 2024

This pull requests adds a googlefonts target 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.

@m4rc1e
Copy link
Contributor Author

m4rc1e commented Mar 14, 2024

Friendly ping @rsms

Copy link
Owner

@rsms rsms left a comment

Choose a reason for hiding this comment

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

Thank you for this. I agree that this is a cleaner solution than GF maintaining a fork :•)

Pipfile Outdated
@@ -10,15 +10,15 @@ ufo2ft = "==2.30.0"
fontmake = "==3.5.*"
fontbakery = "==0.10.4"
skia-pathops = "==0.8.*"
gftools = "==0.9.*"
gftools = "*"
Copy link
Owner

Choose a reason for hiding this comment

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

Please pin the version to whatever GF requires.
Accepting "any version" can break CI (it has happened in the past)

misc/makezip2.sh Outdated
@@ -58,7 +58,8 @@ cp build/fonts/var/InterVariable-Italic.ttf "$ZIPDIR/InterVariable-Italic.ttf"
cp build/fonts/static/Inter*.woff2 "$ZIPDIR/web/" &
cp build/fonts/var/InterVariable.woff2 "$ZIPDIR/web/InterVariable.woff2"
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/"
Copy link
Owner

Choose a reason for hiding this comment

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

(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.

Copy link

Choose a reason for hiding this comment

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

(Not affiliated with the Google Fonts team)

Why are there makezip.sh and makezip2.sh? Is the former obsolete?

Copy link
Owner

Choose a reason for hiding this comment

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

@mon-jai something like that, I forget. None of them are intended to be used directly, just to reduce the complexity of the makefile.

Copy link

@mon-jai mon-jai Mar 21, 2024

Choose a reason for hiding this comment

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

Maybe we could delete the obsolete one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

@rsms
Copy link
Owner

rsms commented Mar 22, 2024

Patch for making a separate Inter-VERSION-GoogleFonts.zip file

--- 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

@m4rc1e
Copy link
Contributor Author

m4rc1e commented Mar 26, 2024

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.

@m4rc1e
Copy link
Contributor Author

m4rc1e commented Mar 26, 2024

I also don't mind deleting makezip.sh since it isn't used anywhere in the chain.

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.

@rsms
Copy link
Owner

rsms commented Mar 26, 2024

@m4rc1e Thank you for the update. I'll hold off on merging until I hear back from you.

Makefile Outdated Show resolved Hide resolved
@m4rc1e
Copy link
Contributor Author

m4rc1e commented May 15, 2024

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 . $(VENV) ; and updated gftools. I needed to update gftools because we changed an api.

@rsms rsms merged commit bde6fa1 into rsms:master May 15, 2024
@rsms
Copy link
Owner

rsms commented May 15, 2024

Thank you for all your help @m4rc1e

@m4rc1e
Copy link
Contributor Author

m4rc1e commented May 16, 2024

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.

@rsms
Copy link
Owner

rsms commented May 23, 2024

@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.

$ make -j var
...
. build/venv/bin/activate ; python misc/tools/bake-vf.py build/fonts/var/.Inter-Roman.var.ttf -o build/fonts/var/InterVariable.ttf
unexpected wght 481.0 (expected 500) <fontTools.ttLib.ttFont.TTFont object at 0x101cc0e50> {'opsz': 14.0, 'wght': 481.0}
unexpected wght 562.0 (expected 600) <fontTools.ttLib.ttFont.TTFont object at 0x101cc0e50> {'opsz': 14.0, 'wght': 562.0}
unexpected wght 643.0 (expected 600) <fontTools.ttLib.ttFont.TTFont object at 0x101cc0e50> {'opsz': 14.0, 'wght': 643.0}
unexpected wght 758.0 (expected 800) <fontTools.ttLib.ttFont.TTFont object at 0x101cc0e50> {'opsz': 14.0, 'wght': 758.0}

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.

@m4rc1e
Copy link
Contributor Author

m4rc1e commented May 24, 2024

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.

@rsms
Copy link
Owner

rsms commented May 24, 2024

@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)

@rsms
Copy link
Owner

rsms commented May 24, 2024

@m4rc1e Since I'm not ready to do QA and publish Inter 4.1 yet, here's a GF build of 66647c0 which will be very similar—if not identical—to the final 4.1 release:

Inter-4.1-GoogleFonts.zip

(You can build your own locally like this: make clean && make -j dist_gf)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants