Skip to content

junicode: update to 1.002 and correct license#82044

Merged
timokau merged 5 commits intoNixOS:masterfrom
ivan-timokhin:junicode-1.002
Apr 6, 2020
Merged

junicode: update to 1.002 and correct license#82044
timokau merged 5 commits intoNixOS:masterfrom
ivan-timokhin:junicode-1.002

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
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 8, 2020
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.

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. labels Apr 4, 2020
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.

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.

@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.

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
@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants