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
iosevka: 3.2.2 -> 3.7.1 #99004
Conversation
pkgs/data/fonts/iosevka/bin.nix
Outdated
@@ -1,7 +1,7 @@ | |||
{ stdenv, fetchzip }: | |||
|
|||
let | |||
version = "3.4.6"; | |||
version = "3.6.2"; |
There was a problem hiding this comment.
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;
.
There was a problem hiding this comment.
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.
2e2c955
to
5b10ddf
Compare
Made some changes:
|
There was a problem hiding this 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.
5b10ddf
to
35f1cc6
Compare
There was a problem hiding this 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!
35f1cc6
to
9ce8cf2
Compare
9ce8cf2
to
92416b4
Compare
92416b4
to
37665bc
Compare
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). |
37665bc
to
3a1f3fc
Compare
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' 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
'';
|
No, we can't break it directly on |
Sharing nixpkgs/pkgs/data/fonts/iosevka/update.sh Line 16 in f28c4a1
and what I get when doing
@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 |
9d9eec7
to
5b6c0e8
Compare
Meanwhile, I removed otfcc from the dependencies, added myself as maintainer, and updated the nodePackages update. |
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. |
Possibly. The packages have little in common by now and we already do that with e.g. Firefox. |
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 @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 |
As for the merge conflict, I don't know what to do. It's indeed frustrating. We need to coordinate runs of node's |
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)
I changed the update script, updating nodePackages now, should be done within the hour. |
5b6c0e8
to
7d15aa4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done.
|
||
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 '"') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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=../../../.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
You'll note that the nerdfonts update script you mentioned does not actually cd into its directory. |
Motivation for this change
Both packages were outdated and out of sync with one another.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)