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

noto-fonts: 2020-01-23 -> unstable-2022-04-25 and other changes #170355

Closed
wants to merge 3 commits into from

Conversation

Mathnerd314
Copy link
Contributor

@Mathnerd314 Mathnerd314 commented Apr 26, 2022

Description of changes
  • noto-tools: adjust dependencies to allow running notodiff
  • noto-fonts:
    • 2020-01-23 -> unstable-2022-04-25; various fonts have been added and updated
    • switch to variable fonts (closes Noto-fonts: switch to variable fonts #166953)
    • split out Chrome OS core fonts (Arimo, Tinos, Cousine) to new output croscore-fonts
    • add Emoji to test:
      image
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested NixOS module test noto-fonts.tests.noto-fonts
  • Tested basic functionality of notodiff
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking - the worst case for the croscore change is falling back to an ugly font so I don't think it's necessary
  • Fits CONTRIBUTING.md.

Comment on lines +128 to +130
noto-fonts = noto-pkg.out;
noto-fonts-extra = noto-pkg.extra;
croscore-fonts = noto-pkg.croscore;
Copy link
Member

Choose a reason for hiding this comment

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

nix-env doesn't differentiate the split outputs. So with this change, nix-env -iA noto-fonts would install noto-fonts-extra and croscore-fonts as well. To avoid this, I think it's preferable to split them into separate derivations instead.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, set meta.outputsToInstall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that nix-env is buggy, but who will actually use nix-env? Installing fonts with nix-env doesn't add them to the system font list, you would have to use a hacky workaround like https://discourse.nixos.org/t/how-to-install-fonts-on-a-non-nixos-system/14758.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And outputsToInstall is [ "out" ] so only the base noto fonts will be installed, the hacks will only be needed for croscore and extra.

Copy link
Member

Choose a reason for hiding this comment

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

I think fonts installed by nix-env would be picked up by fontconfig on NixOS systems. The default configuration in the fontconfig package includes the Nix user profile and default profile in its search path.

<!-- nix user profile -->
<dir>~/.nix-profile/lib/X11/fonts</dir>
<dir>~/.nix-profile/share/fonts</dir>
<!-- FHS paths for non-NixOS platforms -->
<dir>/usr/share/fonts</dir>
<dir>/usr/local/share/fonts</dir>
<!-- nix default profile -->
<dir>/nix/var/nix/profiles/default/lib/X11/fonts</dir>
<dir>/nix/var/nix/profiles/default/share/fonts</dir>

Copy link
Member

Choose a reason for hiding this comment

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

Also for non-NixOS systems, all solutions involve adding ~/.nix-profile/share/fonts to the fontconfig search path in one way or another so avoiding nix-env wouldn't help in this particular case.

Copy link
Member

Choose a reason for hiding this comment

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

the hacks will only be needed for croscore and extra.

I believe nix-env doesn't install any additional outputs if an output is selected explicitly which would be the case here.

Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

I'm not sure if there's a simpler solution than analyzeScan, but I think it's fine to merge. Whatever works for the maintainer is great :)

};

analyzeScan = ./analyze_scan.py;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment what this does roughly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It reads all the font information from fc-scan and figures out which fonts are included in the variable fonts and which aren't and need the OTF version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There probably is a way to do it with shell scripting but my head hurt thinking about it.

@Mathnerd314
Copy link
Contributor Author

I was wondering if these fonts would have an issue similar to #171976, but I tested it and it seems these variable fonts work fine in Discord.

install -m444 -Dt $out_ttf hinted/*/*-${weights}.ttf
fc-scan ./unhinted -f '"%{fullname[0]}","%{file}"\n' > scan.csv
python $analyzeScan
local out_ttf=$out/share/fonts/google-noto
Copy link
Member

Choose a reason for hiding this comment

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

Fonts are no longer installed to the truetype subdirectory. This seems like it would break at least koreader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Freetype scans all subdirectories regardless of name so the directory names are arbitrary. koreader uses Freetype so should work fine. Certainly Fedora doesn't bother with the truetype directory anymore, see e.g. Droid sans. Also it would be misleading in this case to call them "truetype" since they are a mixture of opentype (otf) and variable TTFs.

The more questionable aspect is whether koreader supports variable fonts. It seems to have the individual TTF file names names hard-coded. When I run koreader it crashes with the message frontend/ui/font.lua:353: attempt to index local 'face' (a nil value). This could be because it doesn't like the variable fonts I have installed.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my comment was more (or less?) concrete than that: The koreader derivation assumes this at the moment which should be easy to fix:

for i in ${noto-fonts}/share/fonts/truetype/noto/*; do
ln -s "$i" $out/lib/koreader/fonts/noto/
done

Copy link
Contributor Author

@Mathnerd314 Mathnerd314 May 16, 2022

Choose a reason for hiding this comment

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

Ah, I see. koreader should probably fetch the noto fonts itself in a private derivation, like OnlyOffice. Particularly it needs only a few languages and font variants, as compared to the main noto-fonts package which provides all of them. cc @contrun and @neonfuz who can write that patch. (koreader crashes for me so I don't think I can do it)

Copy link
Member

Choose a reason for hiding this comment

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

@a-m-joseph Any chance you could look into this, seeing as you recently touched koreader?

Copy link

@ghost ghost Jun 2, 2022

Choose a reason for hiding this comment

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

@a-m-joseph Any chance you could look into this, seeing as you recently touched koreader?

I can try.

Other than pragmata pro (worth every penny) I don't know much about fonts, and know even less about unicode emojii :)

Oh sorry by "this" I thought you meant the whole PR rather than the possible koreader breakage. Yes, I will figure out how to keep koreader happy.

Copy link

@ghost ghost Jun 2, 2022

Choose a reason for hiding this comment

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

The more questionable aspect is whether koreader supports variable fonts. It seems to have the individual TTF file names names hard-coded. When I run koreader it crashes with the message frontend/ui/font.lua:353: attempt to index local 'face' (a nil value). This could be because it doesn't like the variable fonts I have installed.

No, that means it couldn't find the fonts.

I get the same error above with this PR.

If I revert this PR I don't get the error.

Ah, I see. koreader should probably fetch the noto fonts itself in a private derivation

Yeah, I'm not excited about this but I don't see any other way considering the hardcoded font names in koreader. That project already has an astonishing level of pinning/vendoring going on. I will write the PR to remove the koreader->noto-fonts dependency and have koreader do its own separate fetchurl to get the fonts. I'm leaving town tomorrow morning and if I try to get it done before then I will end up doing a sloppy/rushed job. This will need to wait until mid/late next week. Feel free to ping me or open a bug and assign it to me as a reminder.

My interest in the nixpkgs koreader expression is partly to be able to hack on koreader using nixpkgs for the builds, but also partly to see if nixpkgs can offer a better way to manage the 50+ projects they currently pull in -- sort of analogous to NixWRT. I'm starting to realize that koreader is almost a tiny Linux distribution, just like OpenWRT is. Unfortunately just convincing their CMake machinery to use pre-downloaded tarballs (instead of insisting on downloading them itself) has been so difficult that my enthusiasm for the endeavor is waning.

Copy link

Choose a reason for hiding this comment

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

Unfortunately just convincing their CMake machinery to use pre-downloaded tarballs (instead of insisting on downloading them itself) has been so difficult

I've finally made some real progress on this. Hope to have a draft PR shortly.

Copy link

@ghost ghost Jun 22, 2022

Choose a reason for hiding this comment

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

This seems like it would break at least koreader.

TL;DR: this problem disappears when we we build koreader from source instead of trying to rebundle the Debian binaries.

It turns out that nixpkgs' koreader/default.nix having a dependency upon nixpkgs' noto-fonts is 100% an artifact of the fact that koreader/default.nix is not much more than dpkg -x. The problem disappears as soon as we build koreader from source.

Koreader has a git submodule containing the exact font files they expect (since they hardwire so many things, like the fonts' filenames).

These fonts are, for some reason, not included in the .deb archives that our current koreader/default.nix is using. I suppose this could be in anticipation of Debian policy, although koreader has never been carried by Debian and would need a lot of other more drastic changes in order for them to accept it. So I really don't know why the fonts get dropped when the deb is built.

In any event: my built-from-source koreader solves the problem here.

@ghost
Copy link

ghost commented Jun 2, 2022

Small request: could you please put the version bump in a commit by itself that does nothing else?

Ideally in the first or last commit of the series, but if that isn't possible then in the middle is fine.

This will make things simpler for anybody with local customizations that need to be adapted as a result of this PR merging. They will have an easier time bisecting/cherrypicking.

@Mathnerd314
Copy link
Contributor Author

Mathnerd314 commented Jun 2, 2022

Small request: could you please put the version bump in a commit by itself that does nothing else?

They changed the directory layout between the revisions (the directory phaseIII_only is gone), so I don't see a way to make a version update work without including the new font selection script. And that's pretty much f4cad5a, with a few testing changes thrown in.

I'm leaving town tomorrow morning and if I try to get it done before then I will end up doing a sloppy/rushed job. This will need to wait until mid/late next week.

I ended up working most of this out, Mathnerd314@1ca433a. The resulting package even seems to work on my system (opening the default help / tutorial thing), yay! It still says:

06/02/22-12:04:57 ERROR failed to register crengine font: cannot register font <./fonts/noto/NotoSerif-BoldItalic.ttf>
06/02/22-12:04:57 ERROR failed to register crengine font: cannot register font <./fonts/noto/NotoSerif-Regular.ttf>

though. This means Noto Serif renders in italic if you select it.

But there's no rush, this PR is pretty much stalled while the various variable font issues work themselves out.

@ghost
Copy link

ghost commented Jun 20, 2022

I ended up working most of this out, Mathnerd314@1ca433a.

Hey, thanks so much for doing this. You clearly know about a billion times more about fonts than I do.

I've cherry-picked your commit into #178320 to avoid merge conflicts later; I hope that's okay. If not, let me know, or open a standalone PR with that commit.

In either case, I will investigate the following, which I am also seeing with Mathnerd314@1ca433a:

The resulting package even seems to work on my system (opening the default help / tutorial thing), yay! It still says:

06/02/22-12:04:57 ERROR failed to register crengine font: cannot register font <./fonts/noto/NotoSerif-BoldItalic.ttf>
06/02/22-12:04:57 ERROR failed to register crengine font: cannot register font <./fonts/noto/NotoSerif-Regular.ttf>

@Luflosi
Copy link
Contributor

Luflosi commented Sep 6, 2022

What's missing to get this merged?

@sternenseemann
Copy link
Member

Figuring out what to do about koreader: Is it already broken? Will it be broken by this change? Can we fix it?

@ghost
Copy link

ghost commented Sep 6, 2022

Figuring out what to do about koreader: Is it already broken? Will it be broken by this change? Can we fix it?

The situation with koreader is largely a result of the fact that by using binary packages we are downstream of Debian and inherit their packaging choices.

Upstream pins and vendors the exact fonts they need, as a git submodule, and when you build from source those wind up in the output.

Debian undoes this as part of their packaging.

Then we re-do it as part of our repackaging, but we don't re-do it exactly as upstream intended: instead of putting in the pinned+vendored fonts that upstream intends, we swap in our own noto-fonts package. So when our noto-fonts is bumped we're out of sync with what upstream intends.

@Luflosi
Copy link
Contributor

Luflosi commented Sep 29, 2022

To me it sounds line the koreader package needs to be fixed to pin the correct version of noto-fonts.
Otherwise we will never be able to update noto-fonts.

@sternenseemann
Copy link
Member

We can also opt to mark it as broken and take care of it later.

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

Successfully merging this pull request may close these issues.

Noto-fonts: switch to variable fonts
4 participants