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

Jump to bottom

iosevka, iosevka-bin: 2.3.3 -> 3.2.2 #88533

Merged
merged 2 commits into from Jul 7, 2020
Merged

iosevka, iosevka-bin: 2.3.3 -> 3.2.2 #88533

merged 2 commits into from Jul 7, 2020

Conversation

rileyinman
Copy link
Contributor

Motivation for this change

Iosevka has a new stable release! The build process has remained the same, but several options (such as Iosevka Term being renamed to Iosevka Fixed, as well as individual character style names) have been changed. This may break some custom builds, so I'm not sure if there should be a warning or if we should leave it up to users to update their build options.

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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

asymmetric
asymmetric reviewed May 24, 2020
View reviewed changes
Copy link
Contributor

@asymmetric asymmetric left a comment •
edited

Choose a reason for hiding this comment

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

I think this should be moved to node-packages-v12.json, since Iosevka now requires Node > 12.

For the same reason, we might need

  iosevka = callPackage ../data/fonts/iosevka {
    nodejs = nodejs-12_x;
    nodePackages = nodePackages_12_x;
  };

in all-packages.nix.

pkgs/data/fonts/iosevka/package.json Outdated Show resolved Hide resolved
@rileyinman
Copy link
Contributor Author

Good catch, I'll rebuild it after moving that.

@prusnak
Copy link
Member

prusnak commented May 30, 2020

Please rework your PR. It now has a merge conflict after merging #89184

@rileyinman rileyinman changed the title iosevka, iosevka-bin: 2.3.3 -> 3.0.1 iosevka, iosevka-bin: 2.3.3 -> 3.1.0 May 31, 2020
@rileyinman
Copy link
Contributor Author

@prusnak Rebased to master in order to fix. In the meantime a new update came out, so this no longer updates to 3.0.1 but rather 3.1.0.

@asymmetric
Copy link
Contributor

@rileyinman could you squash your commits following the guidelines?

@rileyinman rileyinman changed the title iosevka, iosevka-bin: 2.3.3 -> 3.1.0 iosevka, iosevka-bin: 2.3.3 -> 3.1.1 Jun 2, 2020
@rileyinman
Copy link
Contributor Author

Commits squashed, in the meantime a bugfix release was made so the PR now updates to 3.1.1.

@rileyinman
Copy link
Contributor Author

Rebased to master to remove node conflict.

@asymmetric
Copy link
Contributor

@rileyinman thanks for squashing :)

AFAIK though, each package should be its own commit. Here are the guidelines - they dont' mention this explicitly, so I guess it's up for interpretation, but that's how I would do it.

@rileyinman rileyinman changed the title iosevka, iosevka-bin: 2.3.3 -> 3.1.1 iosevka: 2.3.3 -> 3.1.1 Jun 4, 2020
@rileyinman
Copy link
Contributor Author

Rebased to master to fix node-packages conflict.

asymmetric
asymmetric reviewed Jun 11, 2020
View reviewed changes
Copy link
Contributor

@asymmetric asymmetric left a comment •
edited

Choose a reason for hiding this comment

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

I haven't tried this because it's trying to build something massive (node?), but LGTM.

Thanks for your work @rileyinman :)

@AluisioASG
Copy link
Contributor

Since this is still open, can you pass --jCmd=$NIX_BUILD_CORES after npm build -- so that it uses the right amount of cores?

@rileyinman rileyinman changed the title iosevka, iosevka-bin: 2.3.3 -> 3.1.1 iosevka, iosevka-bin: 2.3.3 -> 3.2.2 Jun 24, 2020
@rileyinman
Copy link
Contributor Author

@Alan01252 Done—I also updated it to the latest version and rebased to master to fix the merge conflict with node-packages.

asymmetric
asymmetric requested changes Jun 24, 2020
View reviewed changes
Copy link
Contributor

@asymmetric asymmetric left a comment

Choose a reason for hiding this comment

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

The zsh_history file shouldn't have been created, I reckon.

@asymmetric
Copy link
Contributor

@AluisioASG could you point me to the documentation of the --jCmd flag?

@rileyinman
Copy link
Contributor Author

No idea how that got in there, good catch :P

@AluisioASG
Copy link
Contributor

@AluisioASG could you point me to the documentation of the --jCmd flag?

@asymmetric There isn't AFAIK, Iosevka's build system (verda) doesn't even have a source code link. I had to download the package from NPM and look around for a way to control the number of jobs (it defaults to the number of cores in the system and was overwhelming my VM).

@asymmetric
Copy link
Contributor

@rileyinman could you rebase this please? It's a bit unfortunate this hasn't gotten a review from the maintainers yet - a friendly ping :)

@rileyinman
Copy link
Contributor Author

@asymmetric Funny story, you just caught me right as I was doing that!

@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/182

@ttuegel ttuegel merged commit daec48f into NixOS:master Jul 7, 2020
@rileyinman
Copy link
Contributor Author

Thank you!

@rileyinman rileyinman deleted the iosevka-update branch July 7, 2020 21:55
@asymmetric
Copy link
Contributor

asymmetric commented Jul 13, 2020 •
edited

Is it just me or are the variants (Etoile, Aile, ...) not being built?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asymmetric asymmetric asymmetric requested changes

@ttuegel ttuegel ttuegel approved these changes

@jfrankenau jfrankenau Awaiting requested review from jfrankenau

@babariviere babariviere Awaiting requested review from babariviere

@cstrahan cstrahan Awaiting requested review from cstrahan

Assignees
No one assigned
Labels
6.topic: nodejs 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rileyinman @prusnak @asymmetric @AluisioASG @nixos-discourse @ttuegel