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

mkFont derivation for unified font packaging #91518

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Valodim
Copy link
Contributor

@Valodim Valodim commented Jun 25, 2020

Motivation for this change

Font package derivations in nixpkgs aren't particularly organized: They fall into just a handful of patterns, but in a cargo culted rather than organized fashion. Several decisions that should be subject to policy are pretty much taken at random, e.g. whether license files should be packaged, whether woff/woff2/eot/svg files should be packaged, what path to use for each particular font type, etc.

Things done

This PR introduces a mkFont derivation, which specifies a generic installPhase that fits for the majority of font packages. @puzzlewolf and I migrated some ~190 font packages over to it, and most of them are now trivial derivations that contain only pname+version+src+meta. We also did a general cleanup for consistency. Between all those files we dropped some 1k lines of code :)

Obviously we picked the low hanging fruit here, and the remaining font derivations are slightly more complicated to rewrite for mkFont.

Open questions

This PR is a draft currently, and there are still some open questions. In particular I'd appreciate some thoughts on the following points:

  • Should woff/woff2/eot files be packaged? Other distributions seem to eschew these formats, see this fontconfig issue.
  • Some derivations used different outputs for different font formats. I don't do that now, but perhaps it's useful? Either way, mkFont can consistently apply those kinds of policies for whatever font packages it's used for :)
  • Should license files be packaged? This was inconsistent between derivations, and my overall impression was that only OFL fonts had their license shipped - however the OFL explicitly allows (in paragraph two) to simply reference it from machine-readable metadata fields.
  • The installPhase could be made more complicated to offer more features, such as includeOnly or renameFiles directives. I decided to keep it simple for now, derivations can always override whatever they want (or just use mkDerivation, of course).

Anyways, overall I think this is a significant improvement wrt font packaging consistency 🎉

@Valodim
Copy link
Contributor Author

Valodim commented Jun 25, 2020

I created diffs for all derivations, from a their original form to b their mkFont version: https://gist.github.com/Valodim/d17a6bf89d6a52e56d493d3532c6fa30

For each derivation I output the file contents (just find) of the new package, and then a diff (git diff --no-index a/ b/) to the old one. It's fairly easy to spot differences in this format, but I'm sure I missed some issues :) Still, it's easy to see that the overall consistency is much better than before.

@Valodim Valodim marked this pull request as draft June 25, 2020 21:52
@Valodim Valodim changed the title draft: mkFont mkFont derivation for unified font packaging Jun 25, 2020
@Valodim Valodim requested review from dtzWill, f--t and rycee June 25, 2020 21:59
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/guidelines-on-packaging-fonts/7683/3

Comment on lines +53 to +58
find -iname '*.ttc' -print0 | xargs -0 -r install -v -m644 --target $out/share/fonts/truetype/ -D
find -iname '*.ttf' -print0 | xargs -0 -r install -v -m644 --target $out/share/fonts/truetype/ -D
find -iname '*.otf' -print0 | xargs -0 -r install -v -m644 --target $out/share/fonts/opentype/ -D
find -iname '*.bdf' -print0 | xargs -0 -r install -v -m644 --target $out/share/fonts/misc/ -D
find -iname '*.otb' -print0 | xargs -0 -r install -v -m644 --target $out/share/fonts/misc/ -D
find -iname '*.psf' -print0 | xargs -0 -r install -v -m644 --target $out/share/consolefonts/ -D
Copy link
Contributor

Choose a reason for hiding this comment

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

could you replace this with a function to make the install command?
It likely will have to be in bash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean to have a function installFont "*.ttc" $out/share/fonts/truetype instead of the lines here in the end? Or just one overall?

I left things simple and in one place to keep everything as approachable as possible, at least for development and an initial round of feedback. We can definitely extract some stuff, but we should also keep in mind that more magic also means less self-documenting.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean to have a function installFont "*.ttc" $out/share/fonts/truetype

Yeah, that sounds right to me, but just in the installPhase.
Personally, I'm seeing the same command copy pasted 6 times with only two things changed.

hasMultipleSources = lib.hasAttr "srcs" args;
in
stdenvNoCC.mkDerivation ({
dontPatch = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn’t it make sense to instead provide defaults that can be overridden here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything can be overridden via the // args below, and I do that in a couple of derivations (e.g. dontBuild = false; and provide a build phase on some). Did you have another mechanism for that in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're correct, thanks for the clarification.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but this is inconsistent with other builders so people might be confusing when e.g. patches do not work. It fits more in with trivial builders. Perhaps it should be named installFontFiles and be part of trivial builders.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 26, 2020

There is a problem in that mkFont doesn't handle multiple outputs. Until Pango is released with a fix to this issue, putting bdf/pcf fonts in the same output as otb will effectively break both in most GTK applications.

@Valodim
Copy link
Contributor Author

Valodim commented Jun 26, 2020

That's exactly the type of input I've been looking for, thanks! Coincidentally, there was a comment on that thread yesterday saying it was fixed. Let's stay tuned on whether that's actually true.

Provided the bug is fixed, do you still think multiple outputs would be useful? I can think of a couple arguments for and against (short on time right now, gonna pick this up later).

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 26, 2020

Yes, they could still be useful. For example, there is pkgs.unscii which is huge: it contains rarely used formats live svg, woff and hex that I've split into an extra output.

@jtojnar
Copy link
Member

jtojnar commented Jun 26, 2020

Is this meant just for installing already built font files – I mean it does not make sense for fonts that have Makefiles.

@Valodim
Copy link
Contributor Author

Valodim commented Jun 26, 2020

Is this meant just for installing already built font files – I mean it does not make sense for fonts that have Makefiles.

It's not. It is essentially a specialized version of mkDerivation with a custom installPhase that works well for fonts. I disabled most phases by default as most font packages don't need them, but they can be enabled with no caveats and there are a couple of derivations in the second commit that do that already.

Yes, they could still be useful. For example, there is pkgs.unscii which is huge: it contains rarely used formats live svg, woff and hex that I've split into an extra output.

Right. At the moment, there are lots of derivations that package their woffs, eots, svgs, depending on which other derivation they were cargo-culted from. There are also txts or pdfs with various content, I even saw an xlsx in there.

The point here is to allow us to make decisions about font packaging ("where do ttfs go?", "what chmod should font files have?", "how is fontconfig called?", "should woffs go in a separate output?") as a policy, that can be encoded in mkFont and documented.

I don't particularly care what those policies are, but the more consistent we can be, the more stable assumptions can be made by packagers as well as users :) See also the related discourse thread.

'';
*/

installPhase = ''
Copy link
Member

Choose a reason for hiding this comment

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

This version of installPhase doesn't install .eot fonts (Your diff/gist shows at least one package where the .eot files are no longer there. The commented out installPhase above does include .eot, so I assume this is an accidental omission).

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's not, actually :) eot files are supported in Internet Explorer 6 to 11, but no other browser whatsoever. IE9 (released 9 years ago) and upwards support OTF, TTF, and WOFF, so the only reason to ever ship eot files is compatibility with IE6 to IE8. I suspect that the packages that include eot files don't do so out of careful consideration.

I propose we omit eot files from nix packaging.

(Of course, decisions such as this should be annotated in mkFont to make it easy to discover for packagers who look up how things work, and also in the manual. The "needs documentation" tag definitely applies to PR, I'll try to get around to it so we have a better basis for discussion soon ;)

Copy link
Member

Choose a reason for hiding this comment

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

Looks like .woff and .woff2 fonts aren't handled either.

@asdf8dfafjk
Copy link
Contributor

Incidentally I had something very similar in my local repo (because I love using legible fonts)- would you mind adding a parameter, say- toDelete?[] and adding in installPhase, the following-

			for de in $toDelete
			do
				find -name $de | xargs rm
			done

This is because fontconfig is really bad in determining which fonts from within a family take prececdence (by default). For example, I use the following for Delicious font; if you don't use the toDelete field, then based on which stars align, you may get Delicious Small caps as the applicable font when you just say, "Delicious", or worse Delicious Heavy.

				generic-font-zip
				{
					name = "DeliciousRoman";
					url = "http://www.exljbris.com/dl/delicious-123.zip";
					sha256 = "1lhfnyzfaibz53syfghll3sjiys7x7hvppxx4wbm5gwdgj7cxmg9";
					toDelete = [ "Delicious-Heavy.otf" "Delicious-SmallCaps.otf" ];
				}

Btw, this must have been such a herculean undertaking- our collective eyes will thank you when we can use more legible fonts :).

@Thra11
Copy link
Member

Thra11 commented Aug 6, 2020

I use the following for Delicious font; if you don't use the toDelete field, then based on which stars align, you may get Delicious Small caps as the applicable font when you just say, "Delicious", or worse Delicious Heavy.

What would this mean for people who actually want to use Delicious SmallCaps or Delicious Heavy? Would you put them in a completely separate package?

I was wondering whether a font package could have multiple-outputs, e.g. delicious.regular, delicious.heavy, delicious.smallcaps. As far as I'm aware output names are technically just arbitrary: the fact that many functions and derivations treat outputs like bin, lib, and dev specially doesn't stop you creating your own outputs. I don't know whether this would be considered undesirable abuse of multiple outputs.

However, I think only installing some of the styles available is at best a workaround. Ideally we should try to fix the actual issue. I found this, which is an interesting exploration of some of the complexities of fontconfig font matching. Maybe we can work out why fontconfig is choosing the wrong style from the delicious family and either patch the delicious files to correct the metadata (if that is the issue), or add something to /etc/fonts/conf.d/* to nudge fontconfig in the right direction.

@asdf8dfafjk
Copy link
Contributor

This is not a Nix specific problem, I used to face this in Arch too. Fontconfig has a lot of problems. Specially so if you want to use non-latin fonts. There is no consistent theme underlying fontconfig configuration language is what I've concluded after I've spent a lot of hours on fontconfig, the best bet is to just get do what is needed and spend time doing work.

@Thra11
Copy link
Member

Thra11 commented Aug 7, 2020

In this case, I think you could argue that it is an "upstream" problem/bug. Based on the appearance of the individual styles in the family, it looks like Delicious-Roman.otf should be the "normal"/regular/default variant, but the style data in the font files is as follows:

Delicious-Roman: family: "Delicious", style: "Roman"
Delicious-Heavy: family: "Delicious Heavy" "Delicious", style: "Regular" "Heavy"
Delicious-SmallCaps: family: "Delicious SmallCaps" "Delicious", style: "Regular" "SmallCaps"

It doesn't seem unreasonable that fontconfig favours the heavy and smallcaps styles, as they claim to be "Regular" fonts, which are in the "Delicious" family. Delicious-Roman, on the other hand, claims to be a "Roman" style, which nobody asked for.

I believe there are a few possible workarounds/fixes:

  • Don't install the heavy and smallcaps styles: The simplest solution if it's just for you, but probably not suitable if packaging the font for other to use.
  • Patch the font to make the style metadata better match the actual fonts and their intended usage: Plausible, but not trivial as we don't compile the fonts ourselves.
  • Add a snippet to /etc/fonts/conf.d: NixOS already has things like fonts.fontconfig.localConf for users to add their own rules. If we are going to have something like mkFont, could it maybe allow font packages to specify fontconfig configuration snippets which get concatenated and written to a file with suitable priority in /etc/fonts/conf.d? This would mean that for problematic font families like this, you could write a short snippet to tell fontconfig that "Delicious Roman" is the preferred font in the "Delicious" family.

As you can probably tell, I'm leaning towards the last option, though I haven't tried to implement it yet, and I don't know how common fonts with messed-up metadata are yet.

@asdf8dfafjk
Copy link
Contributor

I agree with your argument

@asdf8dfafjk
Copy link
Contributor

Hello, how I can help you progress this PR?

@ryantm ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020
@asdf8dfafjk
Copy link
Contributor

"little" sad that somebody changed 189 files and rather than helping push this PR, people comments irrelevant comments about code style.

Now this PR is dead. Good job everyone. (some of the usual suspects obviously)

@stale
Copy link

stale bot commented Sep 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 3, 2021
@drupol
Copy link
Contributor

drupol commented Jun 6, 2023

Hi!

I think this PR is very interesting, do you think you could refresh it so we can give it a review?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 6, 2023
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.