-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
line-awesome: init at 1.3.0 #83537
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
line-awesome: init at 1.3.0 #83537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on NixOS unstable.
|
||
postFetch = '' | ||
mkdir -p $out/share/fonts | ||
unzip -j $downloadedFile ${version}/fonts/\*.ttf -d $out/share/fonts/truetype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary? Shouldn't fetchzip
take care of the unpacking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing!
Hm. I modeled the PR after #78409 and other fonts do the same thing, e.g. yanone-kaffeesatz or ubuntu-fonts. But that doesn't mean it's correct 🤔
I think this overrides the postFetch
defined by fetchzip
, which is where the unpacking takes place. So it has to be done manually. But I concur, it's weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems very weird. If we're unzipping manually anyway, we might as well use fetchurl
.
Maybe this should use mkDerivation
instead, disable the unnecessary phases (mostly build) and do the installation in installPhase
. I don't have much experience with fonts in nixpkgs though. @marsam, what are your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Its possible I won't be able to react to emails over the rest of the weekend. So if someone else wants to merge, no need to wait for my approval)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@puzzlewolf according to the docs, you should just add unzip
to nativeBuildInputs
, see: https://nixos.org/nixpkgs/manual/#ssec-unpack-phase .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doronbehar Do you want me to model this after e.g. nafees? It's quite different from e.g. ubuntu-fonts, it uses mkDerivation
instead of fetchzip
. Do you know of a community-agreed default way of doing this kind of font packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that we have many fonts installed with fetchzip
which is a bit unfortunate since this has the disadvantage that it makes it impossible to override these derivations with <font>.overrideAttrs
. nafees is written great so yes, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, now it uses mkDerivation.
|
||
postFetch = '' | ||
mkdir -p $out/share/fonts | ||
unzip -j $downloadedFile ${version}/fonts/\*.ttf -d $out/share/fonts/truetype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@puzzlewolf according to the docs, you should just add unzip
to nativeBuildInputs
, see: https://nixos.org/nixpkgs/manual/#ssec-unpack-phase .
c7ccf1f
to
4b06f32
Compare
And now it's fixed-output. It should be, right? The manual seems pretty clear: https://nixos.org/nix/manual/#sec-advanced-attributes, but fixed-output derivations seem to be their own can of worms NixOS/nix#2270. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much cleaner now!
It shouldn't be fixed output though. Fixed output is a tool that should only be used when needed, as it has many downsides (as you noted yourself). It isn't needed in this case.
4b06f32
to
8930a67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks a lot for the contribution, your receptiveness to feedback and your patience.
If there's anything else you would like me to take a look at (a PR of yours or one you have reviewed), let me know. No guarantees of course. |
Thanks for reviewing, explaining and your receptiveness on discourse 🚀. Currently, there is only #83028, which should probably also go the I wonder if we should document somewhere in the manual that |
Yes, I think #83028 could be modeled after this PR. Documentation would be great! Are you up to writing that? I think the ´fetchX` functions are pretty much just intended to be used as part of a package, but never as the top-level package. If you feel up to it, you could write some documentation. We can then look for some people who have contributed |
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/1 |
Motivation for this change
Line Awesome is a free alternative to Font Awesome, in line icon style.
https://icons8.com/line-awesome/
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)