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

junicode: update to 1.002 and correct license #82044

Merged
merged 5 commits into from Apr 6, 2020

Conversation

ivan-timokhin
Copy link
Contributor

@ivan-timokhin ivan-timokhin commented Mar 8, 2020

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
  • 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/)
    The fonts at least look correct in fontforge.
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    Decreased from 3.0M to 2.2M
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
    meta.maintainers is not set; other than that, I think everything fits.

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
Copy link
Contributor

@puzzlewolf puzzlewolf left a 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 :)

@ivan-timokhin
Copy link
Contributor Author

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.

Copy link
Contributor

@puzzlewolf puzzlewolf left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM 👍

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/134

Copy link
Member

@timokau timokau left a 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.

@@ -1,20 +1,21 @@
{ lib, fetchzip }:

fetchzip {
name = "junicode-0.7.8";
name = "junicode-1.002";
Copy link
Member

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:

Suggested change
name = "junicode-1.002";
pname = "junicode";
version = "1.002";

Copy link
Member

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}";

Copy link
Contributor Author

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.

pkgs/data/fonts/junicode/default.nix Outdated Show resolved Hide resolved
pkgs/data/fonts/junicode/default.nix Outdated Show resolved Hide resolved
pkgs/data/fonts/junicode/default.nix Outdated Show resolved Hide resolved
@timokau
Copy link
Member

timokau commented Apr 4, 2020

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.

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;
};
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this?

@timokau timokau changed the title Junicode: update to 1.002 and correct license junicode: update to 1.002 and correct license Apr 4, 2020
@timokau
Copy link
Member

timokau commented Apr 6, 2020

Perfect. Thank you and welcome to the team :)

@timokau timokau merged commit 1af2f11 into NixOS:master Apr 6, 2020
@ivan-timokhin ivan-timokhin deleted the junicode-1.002 branch December 5, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants