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

nerdfonts: reuse individual fonts #86459

Closed
wants to merge 1 commit into from

Conversation

@jluttine
Copy link
Member

jluttine commented May 1, 2020

Motivation for this change

Previous implementation installed all selected fonts under the same store path and created a new store path for each different selection of fonts. Therefore, when making changes to fonts argument, a new path containing all the selected fonts was created. This causes unnecessary re-building of fonts and disk space usage. This change puts each font under separate store paths and just combines them with symlinkJoin. Now, making changes to fonts argument causes only
rebuilding of a very light wrapping package.

Building example:

nix-build -E 'with import <nixpkgs> {}; nerdfonts.override {fonts=["Iosevka" "Arimo"];}'

cc maintainer @doronbehar

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@jluttine jluttine force-pushed the jluttine:reuse-individual-nerdfonts branch from 85e508a to e4c2a05 May 1, 2020
@jluttine jluttine requested a review from doronbehar May 1, 2020
Copy link
Contributor

doronbehar left a comment

I guess this makes a big difference if you change your fonts a lot. Me, and I assume most people use only 1 or 2 fonts pretty consistently. Never the less, this change is harmless for us and it makes the derivation even smarter so 👍.

Previous implementation installed all selected fonts under the same store path
and created a new store path for each different selection of fonts. Therefore,
when making changes to `fonts` argument, a new path containing all the selected
fonts was created. This causes unnecessary re-building of fonts and disk space
usage. This change puts each font under separate store paths and just combines
them with `symlinkJoin`. Now, making changes to `fonts` argument causes only
rebuilding of a very light wrapping package.
@jluttine jluttine force-pushed the jluttine:reuse-individual-nerdfonts branch from e4c2a05 to 5c158af May 2, 2020
@jluttine
Copy link
Member Author

jluttine commented May 2, 2020

I'll make one more improvement suggestion in a few minutes..

@jluttine
Copy link
Member Author

jluttine commented May 2, 2020

@doronbehar I made one more commit suggestion: add each individual font available as an attribute of nerdfonts. So one can install nerdfonts.agave or nerdfonts.source-code-pro instead of (nerdfonts.override {fonts=["Agave"];}) and so on. A bit of simplicity. Also, I was hoping that nix search would now find these attributes so if one searched nix search iosevka, one would get a result that included nerdfonts.iosevka. However, it doesn't seem to work with what I did and I'm not sure how to make that work.. I think it'd be really nice for the user experience. Otherwise, users need to know that a) nerdfonts is the right package, b) there's fonts argument which needs to be (or can be) overridden and c) how is the name of the font written exactly in nerdfonts repository. This doesn't break anything that worked before, because I just used passthru.

@jluttine
Copy link
Member Author

jluttine commented May 2, 2020

Another option would be to add those to top-level in all-packages.nix. I noticed that there already is terminus-nerdfont and inconsolata-nerdfont.

@doronbehar
Copy link
Contributor

doronbehar commented May 2, 2020

Another option would be to add those to top-level in all-packages.nix. I noticed that there already is terminus-nerdfont and inconsolata-nerdfont.

Oh no please don't do that! These were temporary "fixes" (kept for backwards compatibility) to the problem that nerdfonts were much more annoying to install before #84858 . Thanks to you that change is taken once step further :).

@jluttine
Copy link
Member Author

jluttine commented May 2, 2020

Oh no please don't do that!

Ok! 👍 What do you think about the attributes via passthru? Do you know if it's possible to make them such that nix search finds them?

Copy link
Contributor

doronbehar left a comment

@jluttine I have mixed feelings now regarding this PR... I didn't mind it much in it's previous state where the only real change was to use symlinkJoin and to reuse font packages. Considering what I commented at the review, I think you have 3 options:

  1. Close the PR (do nothing).
  2. Revert it back to use symlinkJoin - in the form in which I approved it initially.
  3. Write an even broader change that will make nerdfonts be an attribute set such as pythonPackages.

But, if you'll go with (3), you'll have to come up with a dictionary such as the one I wrote I don't like, and I doubt you'll find an elegant way to automate it's creation such as in #84979 (comment) .

Regarding the initial motivation for this PR:

Previous implementation installed all selected fonts under the same store path and created a new store path for each different selection of fonts. Therefore, when making changes to fonts argument, a new path containing all the selected fonts was created. This causes unnecessary re-building of fonts and disk space usage.

I'm just curios, are you sharing these nerdfonts derivations between machines and selecting different but exclusive fonts between each of these machines? When I think about it, I don't even see the symlinkJoin implementation making a big difference for the average user. I mean if you just add a nerdfonts.override derivation in your fonts.fonts and leave it there..

# Make each font available also as attributes of nerdfonts package. That is,
# individual font packages can be accessed as nerdfonts._3270,
# nerdfonts.agave, nerdfonts.anonymous-pro and so on.
passthru = fontPkgs;

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 2, 2020

Contributor

Interesting.... This doesn't really work if you try e.g:

nix-build -A nerdfonts.agave

And you haven't built nerdfonts with "Agave" ever..

I'm afraid this also explains the issue of these fonts not found in nix search -u nerdfonts.

"Tinos" = "tinos";
"Ubuntu" = "ubuntu";
"UbuntuMono" = "ubuntu-mono";
"VictorMono" = "victor-mono";

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 2, 2020

Contributor

I don't like this at all, sorry. Part of the point of #84858 was to also automate the process of fetching the available fonts there. This is a hand made dictionary I see no way of automating.

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 2, 2020

Contributor

Correction: #84858 -> #84979

This comment has been minimized.

Copy link
@jluttine

jluttine May 2, 2020

Author Member

Yes, I totally understand, and agree. This could also be automated by some rules in how to do the conversion but they'd be a bit complex and there would still be some exceptions (DejaVu is converted to dejavu instead of deja-vu).

@doronbehar
Copy link
Contributor

doronbehar commented May 2, 2020

users need to know that a) nerdfonts is the right package, b) there's fonts argument which needs to be (or can be) overridden and c) how is the name of the font written exactly in nerdfonts repository.

Officially that should go to Chapter 16: https://nixos.org/nixpkgs/manual/#chap-packages

But, I'd spare my self the pain of docbook since Nixpkgs / Nix is in the process of changing documentation format, see NixOS/rfcs#64 .

@jluttine jluttine force-pushed the jluttine:reuse-individual-nerdfonts branch from 03b2afa to 5c158af May 2, 2020
@jluttine
Copy link
Member Author

jluttine commented May 2, 2020

I chose (2) and reverted back to the original PR. Yeah, I think a proper attributeset like pythonPackages would be the other options, but like you said, automating that is nontrivial and the added value is rather minimal, basically non-existent, only some possible ease of use.

So, this should be now in the state you originally approved (some minor fixes to comments and meta attribute), unless I messed something up.

@doronbehar
Copy link
Contributor

doronbehar commented May 2, 2020

Could you please answer:

Regarding the initial motivation for this PR:

Previous implementation installed all selected fonts under the same store path and created a new store path for each different selection of fonts. Therefore, when making changes to fonts argument, a new path containing all the selected fonts was created. This causes unnecessary re-building of fonts and disk space usage.

I'm just curios, are you sharing these nerdfonts derivations between machines and selecting different but exclusive fonts between each of these machines? When I think about it, I don't even see the symlinkJoin implementation making a big difference for the average user. I mean if you just add a nerdfonts.override derivation in your fonts.fonts and leave it there..

?

@jluttine
Copy link
Member Author

jluttine commented May 2, 2020

Could you please answer:

Regarding the initial motivation for this PR:

Previous implementation installed all selected fonts under the same store path and created a new store path for each different selection of fonts. Therefore, when making changes to fonts argument, a new path containing all the selected fonts was created. This causes unnecessary re-building of fonts and disk space usage.

I'm just curios, are you sharing these nerdfonts derivations between machines and selecting different but exclusive fonts between each of these machines? When I think about it, I don't even see the symlinkJoin implementation making a big difference for the average user. I mean if you just add a nerdfonts.override derivation in your fonts.fonts and leave it there..

?

I'm not sure I understood the question, but I'll try to answer: On my machine, I might add/remove fonts when I need or don't need something. It might be related to development that I try different fonts or I might just want to experiment with different styles in my desktop environment, some applications or text editors. If I had, for instance, Noto Nerdfont (889 MB) installed and then I add some other font (e.g., Iosevka), I would get a new store path which has Noto again, so I'd have 889MB in my store twice. If I don't have the source for Noto anymore in store, nix would also need to download Noto zip again, causing a lot of unnecessary bandwidth usage.

I could, of course, create overridden derivation separately for each font, like:

fonts = [
    ...
    (nerdfonts.override {fonts = ["Noto"];})
    (nerdfonts.override {fonts = ["Iosevka"];})
    ...
];

Then, they can be added and removed independently. So, it is possible to avoid the problem if one knows how.

Another possible (not sure about this) small point (definitely not the main point) is that if I had:

fonts = [
    ...
    (nerdfonts.override {fonts = ["Noto", "Iosevka"];})
    (nerdfonts.override {fonts = ["Iosevka"];})
    ...
];

There would be conflicts because these derivations have different files with the same name for Iosevka (the files have identical content obviously). With this PR, the files are symlinks pointing to the exact same file, so nix can solve that trivially (I'd expect that nix is smart and wouldn't complain about conflicts in that case, but I have no idea if it is so). Anyway, this was more of a side thought, I don't really know how it works in nix.

I don't think this PR makes a big difference in most cases, but I think it doesn't cause any problems and in some cases it can help a little bit or even a lot. I was working on this originally before I noticed that you had already done the main effort of making it possible to install individual fonts without needing to download the full repository. I was doing the same thing, but slightly differently, so I then thought I could make a PR of this small difference that I had in my approach.

@jluttine
Copy link
Member Author

jluttine commented May 3, 2020

@doronbehar I started thinking if I should still implement the option (3) you mentioned: make nerdfonts an attribute set similarly as pythonPackages. Personally, I would prefer this solution the most, but I think it isn't possible to keep it backwards compatible so that nerdfonts installs all nerdfont fonts. Unless, I give a different name for the attribute set (e.g., nerdfontsPackages) and make nerdfonts such that it symlink joins all packages in nerdfontsPackages attribute set. Not sure whether to break backwards compatibility or introduce nerdfontsPackages or if there's some better solution. Personally, I'd just break backwards compatibility and make nerdfonts an attribute set.

Regarding automating the attribute names: I don't think that needs to be automated necessarily. If it is made so that if there's no defined mapping from the font name (e.g., SourceCodePro) to an attribute name (e.g., source-code-pro), an error will be raised when running nix-build -A nerdfonts, then it's easy to detect those when updating the package. The manual fix requires very little work: just write the attribute name for the new font that has been added since the previous release. This amount of manual work is very typical in nixpkgs derivations. It's not comparable to manually checking if new fonts have been added to the release page or updating sha256 of each font, which both you have brilliantly automated. So, I don't see it as a problem, if the mapping from the font name to an attribute name needs to be maintained manually because the need for the need for the tiny amount of manual work can be detected automatically (thanks to your efforts).

Why would I prefer this kind of solution?

  1. Package searches will give you those nerdfonts packages when searching for a font name (e.g., nix search iosevka).
  2. Installing a single font is easy and doesn't require overriding, which is more hidden and non-trivial solution.
  3. It would automatically guide users to install only those fonts that they want instead of installing all nerdfonts by default when they are interested in only one or a few of them.
  4. If someone wants to install all nerdfonts, it could still be done as lib.attrValues nerdfonts. So, there's a simple solution for this usecase too.
  5. EDIT: Conceptually, I think each font in nerdfonts is a separate package similarly as all other fonts are separate packages too.
  6. EDIT: Some code completion tools can list the fonts because they are proper attributes.

But if you feel that you wouldn't approve this kind of solution, then I won't of course waste my time on it. Just wanted share these thoughts.

@doronbehar
Copy link
Contributor

doronbehar commented May 3, 2020

I'm not sure I understood the question, but I'll try to answer: On my machine, I might add/remove fonts when I need or don't need something. It might be related to development that I try different fonts or I might just want to experiment with different styles in my desktop environment, some applications or text editors.

I just don't understand why can't you settle down on a set of fonts and forget about it...

My overall thoughts are that this doesn't worth the effort. There are so many things worth improving in Nixpkgs which will require Nix skills such as yours :). Here are a few examples I think about instantly:

@jluttine
Copy link
Member Author

jluttine commented May 3, 2020

Probably at some point I will settle down with some set of fonts after I don't do any development work that involves trying out different fonts. But anyway, as I said, I figured out already a workaround which makes each font independent:

fonts = [
    ...
    (nerdfonts.override {fonts = ["Noto"];})
    (nerdfonts.override {fonts = ["Iosevka"];})
    ...
];

It just took me some time to figure this out, so I thought it'd be worth making it simpler for other users. I can live with this for sure, no problem. Sometimes it's the small glitches that annoy the most, that's why I thought the attribute set approach would have value for other users, especially non-advanced users (see the points 1-6 in my previous comment).

But ok, I'll work on something else, and won't spend time on this anymore. Feel free to close this (or merge if the current state was anyway ok).

@doronbehar doronbehar closed this May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.