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.

Sign up for GitHub

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

Jump to bottom

iosevka: 1.4.2 -> 1.11.4 #24019

Merged
merged 1 commit into from Mar 18, 2017
Merged

iosevka: 1.4.2 -> 1.11.4 #24019

merged 1 commit into from Mar 18, 2017

Conversation

winniequinn
Copy link
Contributor

@winniequinn winniequinn commented Mar 18, 2017 •
edited

@cstrahan

Motivation for this change

1.11.4 contains numerous improvements including ligatures and fixes to font weights.

Things done

I switched "default.nix" from using fetchFromGitHub to simply pulling down the release as a zip via fetchurl. This seems like the best approach given that the repositoriy no longer contains font files and the alternative would be to build them from scratch.

This is my first time submitting a PR to nixpkgs, so please bonk me if I made any mistakes!

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@winniequinn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cstrahan, @ericsagnes and @leenaars to be potential reviewers.

ndowens
ndowens suggested changes Mar 18, 2017
View reviewed changes
Copy link
Contributor

@ndowens ndowens left a comment

Choose a reason for hiding this comment

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

fetchFromGitHub should be used, not fetchurl here

@winniequinn
Copy link
Contributor Author

winniequinn commented Mar 18, 2017 •
edited

Got it! I'll fix things and poke you when it's ready.

@winniequinn
Copy link
Contributor Author

@ndowens

A few things:

  • I looked into doing it with fetchFromGitHub. It would require nodejs and ttfautohint as dependencies, as well as otfcc which is not currently packaged.

  • I noticed other packages for GitHub-hosted fonts use the same fetchurl approach I am suggesting here, e.g. the popular fira-code and hasklig.

  • The packages that are using fetchFromGitHub, e.g. source-code-pro, are easily able to do so because the associated GitHub repositories contain font files. This is not the case with fonts like Hasklig and present-day Iosevka. (It was, however, the case for Iosevka 1.4.2.)

Given the above, do you still think that using fetchFromGitHub and building the fonts from scratch is required? I am not sure what the advantage would be in this particular case, especially given that many people, including myself, are already using the fonts released by the author and have verified that they work correctly.

@winniequinn
iosevka: 1.4.2 -> 1.11.4
7dccfd4 
1.11.4 contains numerous improvements including ligatures and fixes to
font weights.

I switched "default.nix" from using `fetchFromGitHub` to simply pulling
down the release as a zip via `fetchurl`. This seems like the best
approach given that the repositoriy no longer contains font files and
the alternative would be to build them from scratch.
@ndowens
Copy link
Contributor

ndowens commented Mar 18, 2017

fetchFromGitHub should not require extra deps, it is just a method for getting src from github

@winniequinn
Copy link
Contributor Author

winniequinn commented Mar 18, 2017 •
edited

@ndowens Right, but the issue is that the source does not contain font files: Only the release zips uploaded by the author contain font files. If I use fetchFromGitHub, I need all of the dependencies in order to build the fonts from scratch. If I use fetchurl, I can simply download the fonts that have already been built and tested.

@ndowens
Copy link
Contributor

ndowens commented Mar 18, 2017

the tar.gz version doesn't have them?

@7c6f434c
Copy link
Member

@ndowens the new fetchurl gets the file associated with the release, not the release snapshot per se.

@7c6f434c
Copy link
Member

And there are no tarballs

@7c6f434c 7c6f434c merged commit 084f726 into NixOS:master Mar 18, 2017
@winniequinn
Copy link
Contributor Author

Thanks, @ndowens and @7c6f434c!

@winniequinn winniequinn deleted the iosevka branch March 18, 2017 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndowens ndowens ndowens requested changes

Assignees
No one assigned
Labels
12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@winniequinn @mention-bot @ndowens @7c6f434c @Janik-Haag