-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
ttyd: 1.6.1 -> 1.6.3, generate blob from source #110978
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Please check ofborg status before posting on discourse. I usually just skip red ofborgs entirely. |
The status is only red because I'm not a trusted user, hence ofborg refuses to evaluate the expression. The expression is slightly unusual because it calls yarn2nix at evaluation time. :-) This aspect is what I solicited feedback about. The alternative would be to skip this dynamical generation and just check in a static 300 KiB automatically-generated .nix file. Would also be totally fine with me, just wanted to know which route I should pursue. |
pkgs/servers/ttyd/default.nix
Outdated
let | ||
yarnNixFile = runCommand "yarn.nix" {} | ||
"${yarn2nix}/bin/yarn2nix --lockfile ${src}/html/yarn.lock --no-patch --builtin-fetchgit > $out"; | ||
yarnNix = callPackage yarnNixFile {}; |
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 does import from derivation, which we don't allow in 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.
Hence the evaluation error.
pkgs/servers/ttyd/default.nix
Outdated
let | ||
yarnNixFile = runCommand "yarn.nix" {} | ||
"${yarn2nix}/bin/yarn2nix --lockfile ${src}/html/yarn.lock --no-patch --builtin-fetchgit > $out"; | ||
yarnNix = callPackage yarnNixFile {}; |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Thank you @Mic92 and @SuperSandro2000 for your comments! Accordingly, I removed the dynamic generation and checked in a static yarn.nix file. |
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 is 300KB for a single package? I am against having this in nixpkgs. The problem is that this is also increases nixpkgs evaluation time massively.
To be honest I rather have pre-compiled html than reviewing 10000 lines generate nix files. |
@@ -0,0 +1,10 @@ | |||
#!/usr/bin/env nix-shell | |||
#!nix-shell -i bash -p yarn2nix curl |
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.
#!nix-shell -i bash -p yarn2nix curl | |
#!nix-shell -i bash -p curl gnugrep yarn2nix |
Sorry. For not reading the PR message before putting my comment below. I was asked to have a look at the evaluation error, so I did not pay attention to that. |
I marked this as stale due to inactivity. → More info |
with builtins; | ||
|
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.
with builtins; |
I marked this as stale due to inactivity. → More info |
Heh, I also bumped ttyd a while ago, so the bump could be dropped after a rebase: #123135. Didn't see this PR at the time. I also don't really like the large files in nixpkgs, so a 👎 from me on this. I think this would be better suited to be carried as an overlay if you want to do such things, rather than being checked in to nixpkgs. |
Motivation for this change
The first commit in this pull request is a very ordinary version bump (1.6.1 to 1.6.3).
In the second, we fix a long-time issue with our ttyd package: The file
src/html.h
in the ttyd repository is generated from other source files in thehtml/
subdirectory. Previously, we just used the pre-built file. With this pull request, we are generating it on our own.I believe this to be in the Nix spirit, please correct me if I'm wrong. It also has the practical benefit that overriding this package is now easier -- before, any changes to the
html/
subdirectory wouldn't have any effect.In order to avoid checking in a 300+ KiB
yarn.nix
file into nixpkgs (automatically generated fromyarn.lock
), this pull requests generates it on the fly. Please indicate whether this is weird, if so I'll just check in the generatedyarn.nix
as a static file.The current maintainer @thoughtpolice shouldn't have to bear the work of maintaining my addition. Hence I'm adding myself as maintainer. :-)
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)