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
junicode: update to 1.002 and correct license #82044
Conversation
Both the project page ([1], third paragraph) and the documentation ([2], second-to-last paragraph on the last page) indicate that the font is available under SIL OFL. [1]: http://junicode.sourceforge.net/ [2]: http://junicode.sourceforge.net/Junicode.pdf
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 your first PR to Nixpkgs! The changes look good to me 👍
Would you consider adding yourself as a maintainer of this package? As you have already worked on it :)
I would personally prefer for the maintainer to be a bit more knowledgeable about the inner workings of the whole fontconfig thing, but I guess I can at least keep it up-to-date, so why not. |
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.
Thank you! LGTM 👍
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
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 pretty good to me. My comments aren't really things you introduced, but its always nice to do some cleanup on the lines you're touching anyways.
pkgs/data/fonts/junicode/default.nix
Outdated
@@ -1,20 +1,21 @@ | |||
{ lib, fetchzip }: | |||
|
|||
fetchzip { | |||
name = "junicode-0.7.8"; | |||
name = "junicode-1.002"; |
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.
In the spirit of NixOS/rfcs#35:
name = "junicode-1.002"; | |
pname = "junicode"; | |
version = "1.002"; |
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 just realized that this is using fetchzip
, not mkDerivation
. I'm not sure if the "name from pname and version" behaviour is implemented here. If it isn't, you additionally need to set
name = "${pname}-${version}";
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.
Not only doesn't it support ‘name from pname and version’, it actually gives me an error about unexpected argument ‘version’ in fetchurl if I try. So I let-bound them instead (following the example of agave.
The alternative here is no maintainer, so I'm glad you're willing to pick this up :) Also note that maintainership doesn't have to be exclusive, so if someone else wants to help out in the future they can add themselves too. |
name = "Ivan Timokhin"; | ||
github = "ivan-timokhin"; | ||
githubId = 9802104; | ||
}; |
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.
This should be two commits:
maintainers: add ivan-timokhin
and
junicode: add ivan-timokhin to maintainers
You can change this with an interactive rebase (git rebase -i
, mark this commit as edit, split it into two commits). If you need more help with this, feel free to ask.
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.
Like this?
00a8576
to
04537c7
Compare
04537c7
to
9c7dc00
Compare
Perfect. Thank you and welcome to the team :) |
Motivation for this change
Update Junicode to the latest available version. Among other changes, Greek alphabet has been moved into a separate font, Foulis Greek, included alongside Junicode variants.
While at it, I have noticed that the license in the metadata doesn't match the one on the project site, and changed that as well.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)The fonts at least look correct in fontforge.
nix path-info -S
before and after)Decreased from 3.0M to 2.2M
meta.maintainers
is not set; other than that, I think everything fits.