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: separate into individual font packages, 3.2.1 -> 3.3.0 #354543

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

Conversation

rc-zb
Copy link
Contributor

@rc-zb rc-zb commented Nov 8, 2024

Separate Nerd Fonts into individual packages under the namespace of nerd-fonts.
And update it to 3.3.0.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Mention the package maintainers

@doronbehar


Add a 👍 reaction to pull requests you find important.

@rc-zb rc-zb changed the title Refactor Nerd Fonts nerdfonts: refactor into individual font packages Nov 8, 2024
@rc-zb rc-zb force-pushed the refactor-nerdfonts branch 2 times, most recently from efb304d to 859ddb9 Compare November 8, 2024 18:26
@rc-zb rc-zb marked this pull request as ready for review November 8, 2024 18:29
@rc-zb rc-zb force-pushed the refactor-nerdfonts branch 2 times, most recently from 10ac908 to a607e0c Compare November 8, 2024 19:09
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 9, 2024
@rc-zb rc-zb force-pushed the refactor-nerdfonts branch 4 times, most recently from 4f9938b to c39fc62 Compare November 9, 2024 03:58
Copy link
Contributor

@r-vdp r-vdp left a comment

Choose a reason for hiding this comment

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

Should we not create a top-level attribute for each font, and then an additional one that bundles all of them?

Comment on lines 52 to 89
builtins.listToAttrs (
map (font: lib.attrsets.nameValuePair font.attrName (makeNerdFont font)) (
lib.trivial.importJSON ./fonts.json
)
)
Copy link
Contributor

@r-vdp r-vdp Nov 9, 2024

Choose a reason for hiding this comment

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

Suggested change
builtins.listToAttrs (
map (font: lib.attrsets.nameValuePair font.attrName (makeNerdFont font)) (
lib.trivial.importJSON ./fonts.json
)
)
symlinkJoin {
name = "nerd-fonts";
paths = map makeNerdFont (lib.trivial.importJSON ./fonts.json);
}

Copy link
Contributor

@r-vdp r-vdp Nov 9, 2024

Choose a reason for hiding this comment

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

We need to produce a single derivation here, containing all fonts, right?

Maybe you want to use callPackages instead (note the s at the end), and then additionally add the symlinkJoin to the set to have a derivation containing all fonts.

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 don't think bundling different fonts together (as in the legacy nerdfonts) is a good idea. They have different origins, different descriptions, different licenses and different tarballs. I.e. they have nothing to do with one another except that they live and get patched in the same repo. Should we agglomerate them together, we would have to bundle many of the kdePackages together too, which is so meaningless as well as impractical.

Copy link
Contributor

@r-vdp r-vdp Nov 9, 2024

Choose a reason for hiding this comment

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

I meant that we remove the code for the old derivation altogether, we add the new derivations, and we add a symlinkJoin with all of them as the replacement for the old package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No... Even if we manage to make such a mono-package, either it can't share the interface with its predecessor, or it shares the defeat as well -- difficulty to be built by Hydra due to its too many variants. On the other hand, the user may choose all the fonts by just appending the whole package set to their "fonts" or "packages" list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well no, that's the thing of symlinkjoin, it just symlinks. So the package would be tiny, it just links to all the others.

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 don't know what Hydra actually do to build the cache. But if it was overwhelmed by the old package, which fetched only one GiB or two of tarballs, it must had made a huge number of random compositions on the overridable font choices. For the new package set, however, you can just mix the fonts in any manners as for other packages. Specifying font members and building a dedicated mono-derivation, lightweight or not, is quite redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @rc-zb in this topic. I'll only add that if someone really wants to get all the fonts they can use Nix code to simply take all attrValues of nerd-fonts.

@r-vdp
Copy link
Contributor

r-vdp commented Nov 9, 2024

What's the reason for keeping the old derivation?

@rc-zb rc-zb marked this pull request as draft November 9, 2024 14:43
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: changelog labels Nov 9, 2024
@rc-zb rc-zb changed the title nerdfonts: refactor into individual font packages nerdfonts: separate into individual font packages Nov 9, 2024
@rc-zb rc-zb marked this pull request as ready for review November 9, 2024 16:16
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Beautiful work! Thanks :). I can't believe only now someone has thought about this.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-2411.section.md Outdated Show resolved Hide resolved
pkgs/by-name/ne/nerd-fonts/update.js Outdated Show resolved Hide resolved
pkgs/by-name/ne/nerd-fonts/update.js Outdated Show resolved Hide resolved
@rc-zb rc-zb force-pushed the refactor-nerdfonts branch 3 times, most recently from 9c8ddc3 to 5deea25 Compare November 9, 2024 19:04
pkgs/data/fonts/nerd-fonts/update.js Outdated Show resolved Hide resolved
pkgs/data/fonts/nerd-fonts/default.nix Outdated Show resolved Hide resolved
@rc-zb rc-zb force-pushed the refactor-nerdfonts branch 2 times, most recently from bfc0b4f to fd4877e Compare November 10, 2024 03:24
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

simply vendoring both files - upstream's release SHA-256.txt, and fonts.json, and with pure Nix, generate the derivations

If the font families information were still needed, we would have to download and unpack the tarballs and fc-query the info anyway.

Then I think at this point I would let it go - it is not that important and usually in people's configuration the font families of each font is something that should stay constant in future releases of nerd fonts.

nixos/doc/manual/release-notes/rl-2411.section.md Outdated Show resolved Hide resolved
Comment on lines 52 to 89
builtins.listToAttrs (
map (font: lib.attrsets.nameValuePair font.attrName (makeNerdFont font)) (
lib.trivial.importJSON ./fonts.json
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @rc-zb in this topic. I'll only add that if someone really wants to get all the fonts they can use Nix code to simply take all attrValues of nerd-fonts.

Comment on lines 60 to 89
builtins.listToAttrs (
map (font: lib.attrsets.nameValuePair font.attrName (makeNerdFont font)) (
lib.trivial.importJSON ./fonts.json
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it'd be easier to use lib.genAttrs?

pkgs/data/fonts/nerd-fonts/convert-license.nix Outdated Show resolved Hide resolved
Comment on lines 5 to 8
"Bitstream-Vera AND MIT" = [
bitstreamVera
mit
];
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use strings here,

pkgs/data/fonts/nerd-fonts/convert-license.nix Outdated Show resolved Hide resolved
@rc-zb rc-zb force-pushed the refactor-nerdfonts branch 3 times, most recently from 9ceb003 to 9f6b6f6 Compare November 19, 2024 15:13
@rc-zb rc-zb changed the title nerdfonts: separate into individual font packages nerdfonts: separate into individual font packages, 3.2.1 -> 3.3.0 Nov 19, 2024
@rc-zb
Copy link
Contributor Author

rc-zb commented Nov 19, 2024

Then I think at this point I would let it go

Now it goes.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 20, 2024
@ofborg ofborg bot requested a review from doronbehar November 20, 2024 21:40
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to reply, I'm pretty busy. I also wish I could have helped more directly by simply checking out locally the PR and testing and pushing my suggestions.

pkgs/data/fonts/nerd-fonts/manifests/hashes.json Outdated Show resolved Hide resolved
pkgs/data/fonts/nerd-fonts/update.py Outdated Show resolved Hide resolved
if builtins.match "[[:digit:]].*" version != null then
version
else
"0-unstable-" + builtins.head (lib.strings.splitString "T" releaseInfo.published_at);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the version of the nerd-fonts release tag should be used here, and this would allow you to take slightly less info from that large release info json file.

Copy link
Contributor Author

@rc-zb rc-zb Nov 23, 2024

Choose a reason for hiding this comment

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

This would break the consistency among versions of different fonts. And if the upstream does not want to specify some of the individual versions, why do we care about them?
The size does not matter, because there is only one published_at in the response.

pkgs/data/fonts/nerd-fonts/default.nix Outdated Show resolved Hide resolved
pkgs/data/fonts/nerd-fonts/default.nix Outdated Show resolved Hide resolved
pkgs/data/fonts/nerd-fonts/convert-license.nix Outdated Show resolved Hide resolved
pkgs/data/fonts/nerd-fonts/update.py Outdated Show resolved Hide resolved
pkgs/data/fonts/nerd-fonts/update.py Outdated Show resolved Hide resolved
@rc-zb rc-zb force-pushed the refactor-nerdfonts branch 2 times, most recently from c3f5951 to 7318ffa Compare November 23, 2024 06:16
@rc-zb
Copy link
Contributor Author

rc-zb commented Nov 23, 2024

and the update script could be a simple bash script grabbing the latest release's SHA-256.txt file and fonts.json

So eventually some sort of "algorithms" are still needed for the script? And the checksums JSON become similar to the one before refactoring, except for the removal of nix-prefetch-url...😂

@doronbehar
Copy link
Contributor

and the update script could be a simple bash script grabbing the latest release's SHA-256.txt file and fonts.json

So eventually some sort of "algorithms" are still needed for the script? And the checksums JSON become similar to the one before refactoring, except for the removal of nix-prefetch-url...😂

Not running nix-preferch-url per font was my main objection to the original design of the large JSON file. My original intention now was to make the update script as minimal as possible,, but I observed your insistence on using a "real" programming language, and it came out pretty nice, especially since converting from the original SHA-256.txt file format to some structured form is easier with Python and JSON v.s with Nix (with builtins.readFile), and because it allowed us to filter a bit the information from upstream's files that we vendor.

I pushed a few small improvements to the update.py script, that I think you'd easily agree with. The main big improvement I pushed is the support for the commit feature of update scripts:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#supported-features

Now you can run the script only with nix-shell maintainers/scripts/update.nix. This, and the support for the commit feature, are 2 more good reasons to use a real programming language.

Glad I finally found the time to help with this too :). Waiting to see your approval. Please note how you can git checkout HEAD^, and run:

nix-shell maintainers/scripts/update.nix --argstr path nerd-fonts --argstr commit true

And get the same result.


installPhase =
let
dirName = lib.strings.concatStrings (lib.strings.splitString " " patchedName);
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW Why did you decide to not use folderName here?

Copy link
Contributor Author

@rc-zb rc-zb Nov 24, 2024

Choose a reason for hiding this comment

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

Because folderName is essentially the original name concatenated, not the font (family) name of the released patched font, some of which have to deviate from the former due to their RFN policy. Also, the texts in the preview images on the website are generated from patchedName instead of folderName.

Copy link
Contributor

@l0b0 l0b0 left a comment

Choose a reason for hiding this comment

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

Love it; thank you! After installing pkgs.nerdfonts just to get a single font, and then getting a bit of a shock when looking at nix-tree, this is a welcome change.

pkgs/data/fonts/nerd-fonts/update.py Show resolved Hide resolved
@doronbehar
Copy link
Contributor

Let's see a nixpkgs-review report when CI is green, and merge 🎉.

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 24, 2024
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 24, 2024
@doronbehar doronbehar added the backport release-24.11 Backport PR automatically label Nov 24, 2024
@doronbehar
Copy link
Contributor

Fixed merge conflict in the last push, and labeled the PR so it will be backported to release-24.11.

@doronbehar
Copy link
Contributor

I've also built all nerd-fonts manually (without nixpkgs-review) and it seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 2 This PR was reviewed and approved by two reputable people backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants