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

iosevka: 3.2.2 -> 3.7.1 #99004

Merged
merged 7 commits into from Nov 4, 2020
Merged

iosevka: 3.2.2 -> 3.7.1 #99004

merged 7 commits into from Nov 4, 2020

Conversation

AluisioASG
Copy link
Contributor

Motivation for this change

Both packages were outdated and out of sync with one another.

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.

@@ -1,7 +1,7 @@
{ stdenv, fetchzip }:

let
version = "3.4.6";
version = "3.6.2";
Copy link
Member

Choose a reason for hiding this comment

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

Do iosevka and iosevka-bin always have to be in sync with each other? If so, I think we should ensure they'd always stay the same by changing this line to version = iosevka.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.

They don't need to be, it just happens that iosevka-bin gets updated automatically, while iosevka needs the package.json to be regenerated. It'd make it less confusing if they were updated in lockstep though.

I think I'll borrow the nerdfonts update script to help with that.

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

Made some changes:

  • centralized version number and hashes in srcs.nix
  • filtered package.json to keep only name, version, and dependencies
  • added an update script

Copy link
Member

@midchildan midchildan left a comment

Choose a reason for hiding this comment

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

Great work! There's a single typo that needs fixing, but otherwise looks good.

pkgs/data/fonts/iosevka/update.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@rileyinman rileyinman left a comment

Choose a reason for hiding this comment

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

Sorry it took me a bit to get around to this—the changes look great. The auto-update script especially will be handy, since the update process was a bit tedious and that's one of the reasons I hadn't gotten around to the new version yet. Thanks for the improvements!

pkgs/data/fonts/iosevka/update.sh Outdated Show resolved Hide resolved
pkgs/data/fonts/iosevka/package.json Outdated Show resolved Hide resolved
@AluisioASG AluisioASG changed the title iosevka, iosevka-bin: 3.x.y -> 3.6.2 iosevka, iosevka-bin: 3.x.y -> 3.6.3 Oct 4, 2020
@AluisioASG
Copy link
Contributor Author

Updated to 3.7.0.

@ryantm what's the current procedure for updating node-packages.nix? I've removed the update from this PR but I'm guessing it'll break the ofborg CI run (as well as the automated update once I stop calling generate.sh from the update script).

@AluisioASG AluisioASG changed the title iosevka, iosevka-bin: 3.x.y -> 3.6.3 iosevka, iosevka-bin: 3.x.y -> 3.7.1 Oct 28, 2020
@doronbehar
Copy link
Contributor

OK. It took me a while to grab my head around the changes. It looks good, although it's a bit of a mess. The problem is that many users are probably happy with iosevka-bin since it's easier to compile an override of it if one wishes only to get different / certain variants. Hence it's not suitable to discard the prebuild derivation for the sake of the source build derivation.

Unrelated, for iosevka, it's good to put it's deps in node-packages.json (as it used to be, only previously it was a manually downloaded package.json) so it will (hopefully) get updated frequently by the node packages maintainers (which we currently lack officially).

I was considering maybe suggesting to put the update script of the source build derivation in node-packages' ./generate.sh but that seems like too much to me, and inappropriate though I'm open to argue about it.

I have a few nitpicks left though (appropriate to put in separate commits in this PR):

diff --git i/pkgs/data/fonts/iosevka/bin.nix w/pkgs/data/fonts/iosevka/bin.nix
index c3b7767bccc..853c338e184 100644
--- i/pkgs/data/fonts/iosevka/bin.nix
+++ w/pkgs/data/fonts/iosevka/bin.nix
@@ -1,4 +1,4 @@
-{ stdenv, lib, fetchurl, unzip
+{ stdenv, lib, fetchurl, unzip, iosevka # Only for it's meta
 , variant ? ""
 }:
 
@@ -29,15 +29,7 @@ in stdenv.mkDerivation rec {
     unzip -d $out/share/fonts/truetype $src
   '';
 
-  meta = with lib; {
-    homepage = "https://be5invis.github.io/Iosevka/";
-    downloadPage = "https://github.com/be5invis/Iosevka/releases";
-    description = ''
-      Slender monospace sans-serif and slab-serif typeface inspired by Pragmata
-      Pro, M+ and PF DIN Mono, designed to be the ideal font for programming.
-    '';
-    license = licenses.ofl;
-    platforms = platforms.all;
+  meta = iosevka.meta // {
     maintainers = [ maintainers.cstrahan ];
   };
 
diff --git i/pkgs/data/fonts/iosevka/default.nix w/pkgs/data/fonts/iosevka/default.nix
index 9ef751688db..ef2df780e83 100644
--- i/pkgs/data/fonts/iosevka/default.nix
+++ w/pkgs/data/fonts/iosevka/default.nix
@@ -75,7 +75,7 @@ stdenv.mkDerivation rec {
 
   buildPhase = ''
     runHook preBuild
-    npm run build --no-update-notifier -- --jCmd=$NIX_BUILD_CORES ttf::$pname >/dev/null
+    npm run build --no-update-notifier -- --jCmd=$NIX_BUILD_CORES ttf::$pname
     runHook postBuild
   '';
 
  1. Don't silence the output of npm run build - it's interesting IMO.
  2. Don't repeat yourself (meta information).

@doronbehar
Copy link
Contributor

Should I drop the last commit (that updates node-packages.nix) even if it breaks iosevka on master in the meantime?

No, we can't break it directly on master. If there was an upcoming nodePackages ./generate.sh update, it would have been appropriate to tell the updater to grab the commits from here and we'd close this PR.

@AluisioASG
Copy link
Contributor Author

Don't silence the output of npm run build - it's interesting IMO.

As far as I can tell there is no way to get a useful log without getting progress spam in the way, and any redirects just shut down all logging:

> iosevka@3.7.1 build /build/Iosevka-3.7.1
> verda -f verdafile.js "--jCmd=2" "--verbosity=6" "ttf::iosevka"
⢀⠀ ● Note Invalidating build journal due to self-tracking.
⡀⠀  | CPU:000% - RAM:000%⡀⠀ Complete 04 - Executing 2 - Pending 59 (06%) | CPU:000% - RAM:000%⠄⠀ Complete 004 - Executing 0 - Pending 110 (03%) | CPU:000% - RAM:000%⢂⠀ Complete 059 - Executing 2 - Pending 109 (34%) | CPU:000% - RAM:000%⡂⠀ Complete 062 - Executing 2 - Pending 161 (27%) | CPU:000% - RAM:000%⠅⠀ Complete 063 - Executing 0 - Pending 167 (27%) | CPU:000% - RAM:000%♦ node ./node_modules/patel/bin/patel-c --strict font-src/kits/boole-kit.ptl -o
⢃⠀ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:000% - RAM:000%♦ node ./node_modules/patel/bin/patel-c --strict font-src/kits/spiro-kit.ptl -o
⢃⠀ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:000% - RAM:000%⢃⠀ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:000% - RAM:000%⡃⠀ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:000% - RAM:000%⠍⠀ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:000% - RAM:000%⢋⠀ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:051% - RAM:053%⡋⠀ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:051% - RAM:054%⠍⠁ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:051% - RAM:054%⢋⠁ Complete 068 - Executing 2 - Pending 199 (25%) | CPU:051% - RAM:054%♦ node ./node_modules/patel/bin/patel-c --strict font-src/meta/aesthetics.ptl
^C

Even if I reduce verbosity, it'll keep saving the progress bar updates for printing later.

> iosevka@3.7.1 build /build/Iosevka-3.7.1
> verda -f verdafile.js "--jCmd=2" "--verbosity=5" "ttf::iosevka"

⢀⠀ ● Note Invalidating build journal due to self-tracking.

When that build finished, it produced 7200 lines of the following:
Progress bar spam after build

@AluisioASG
Copy link
Contributor Author

Sharing meta seems like it'll mess up with iosevka-bin's update script, as it locates the file to update with

file=$(nix-instantiate --eval -A iosevka-bin.meta.position | sed -r 's/^"(.*):[0-9]+"$/\1/')

and what I get when doing meta = iosevka.meta // ... is

nix-repl> iosevka-bin.meta.position
"/home/aasg/nix/nixpkgs/iosevka/pkgs/data/fonts/iosevka/default.nix:98"

@deviant is there a problem if I replace that line with the following, or similar (I see you also write to variants.nix further down)?

file=$(dirname "${BASH_SOURCE[0]}")/bin.nix

@AluisioASG
Copy link
Contributor Author

Meanwhile, I removed otfcc from the dependencies, added myself as maintainer, and updated the nodePackages update.

@deviant
Copy link
Member

deviant commented Nov 3, 2020

Sharing meta seems like it'll mess up with iosevka-bin's update script, as it locates the file to update with

file=$(nix-instantiate --eval -A iosevka-bin.meta.position | sed -r 's/^"(.*):[0-9]+"$/\1/')

@deviant is there a problem if I replace that line with the following, or similar (I see you also write to variants.nix further down)?

file=$(dirname "${BASH_SOURCE[0]}")/bin.nix

Not by me. I pretty much just copied that out of the common updater script. Perhaps it would be good to move iosevka-bin into a directory of its own, however? That way the file names can be more normal.

@AluisioASG
Copy link
Contributor Author

Perhaps it would be good to move iosevka-bin into a directory of its own, however? That way the file names can be more normal.

Possibly. The packages have little in common by now and we already do that with e.g. Firefox.

@doronbehar
Copy link
Contributor

Sharing the meta should not be blocked by the design of the update script. You can put the version in a separate file and not use nix-instantiate or whatever it is.

@AluisioASG I'm sorry this got held up, but now we have a merge conflict...

If you could please change the update script to not rely on meta.position and use a safer way to increment the version, it'd be great.

@doronbehar
Copy link
Contributor

As for the merge conflict, I don't know what to do. It's indeed frustrating. We need to coordinate runs of node's ./generate.sh... (cc @cideM).

@doronbehar
Copy link
Contributor

We could also (after the version increment issue will be solved) kindly ask @cideM to grab the commits from here (and perhaps along this commit please 🙏) and then put them both in a separate PR and then run ./generate.sh.

The two packages have the same metadata, except for the maintainers.
Move bin.nix's update script to update-bin.sh so it doesn't conflict
with default.nix's update script.
Previously we used nodePackages only to fetch Iosevka's build
dependencies and then fetched the source code ourselves.  Updates
involved changing the version and hashes in the `iosevka` derivation and
then running node-packages/generate.sh to update the build dependencies,
which in turns updates *all* of node-packages.nix.

A new proposed policy for handling node-packages.nix updates involves
batching those updates.  Previously, that would mean `iosevka` and its
build dependencies could end up out of sync until the batched update was
run.

To work with the new policy, we now fetch Iosevka's source code (and
not only its dependencies) through nodePackages.  Updates are done by
changing the source URL in node-packages.json, which eventually becomes
part of an update of node-packages.nix, which is then propagated to
`iosevka` itself.

One con of this strategy is that errors can not be caught directly
after the update, but only after node-packages.nix is regenerated.
As outlined in the previous commit, we just need to update the source
URL in node-packages.json, and wait for node-packages.nix to be rebuilt
from it.
As of Iosevka 3.7.0, otfcc is no longer used.  I haven't checked if
the situation has changed since [2017] but this should make Iosevka
available on aarch64-linux and *-darwin.

[2017]: NixOS#31835 (comment)
@AluisioASG
Copy link
Contributor Author

I changed the update script, updating nodePackages now, should be done within the hour.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Well done.

@doronbehar doronbehar merged commit 81c5e31 into NixOS:master Nov 4, 2020

release=$(curl -s https://api.github.com/repos/be5invis/Iosevka/releases/latest)

oldVersion=$(nix-instantiate --eval -E 'with import ./. {}; lib.getVersion iosevka-bin' | tr -d '"')
oldVersion=$(nix-instantiate --eval -E 'with import ../../../.. {}; lib.getVersion iosevka-bin' | tr -d '"')
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong; you're supposed to run the update script from the root directory of nixpkgs.

Copy link
Contributor

Choose a reason for hiding this comment

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

The script changes the directory of itself before executing these commands.

Copy link
Member

Choose a reason for hiding this comment

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

It seems, at least, uncommon:

v@february ~/s/nixpkgs (local)> rg -t sh 'import \./' | wc -l
10
v@february ~/s/nixpkgs (local)> rg -t sh 'import \.\./' | wc -l
3

set -euo pipefail
cd "$(dirname "${BASH_SOURCE[0]}")"

nixpkgs=../../../..
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@deviant
Copy link
Member

deviant commented Nov 4, 2020

You'll note that the nerdfonts update script you mentioned does not actually cd into its directory.

@AluisioASG AluisioASG deleted the aasg/iosevka branch November 4, 2020 17:13
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

8 participants