Conversation
github-actions
bot
added
10.rebuild-darwin: 1-10
drupol
left a comment
drupol
left a comment
There was a problem hiding this comment.
Diff LGTM - let's wait for a couple of days to have some other feedback.
wegank
added
the
12.approvals: 1
doronbehar
left a comment
doronbehar
left a comment
There was a problem hiding this comment.
I like this change! A few small comments and I think this should be good to go :).
| # This might be worthwhile to disable if the store lives on a compressed filesystem. | ||
| withCompression ? true, | ||
| # Allow disabling the unicode_start/unicode_stop utilities, which | ||
| # unfortunately pull in bash into the closure. | ||
| withScripts ? true, |
There was a problem hiding this comment.
Maybe we can take the risk and disable these too by default? Disabling these in an overlay would require lots of rebuilds, and being able to use compression and the scripts, is something that would be required for specific applications, and a user that needs these features can build and use a modified version of kbd just for that.
There was a problem hiding this comment.
Other distros (Debian) ship these scripts as well. I would only feel comfortable disabling these by default, if we find out who actually needs them.
Another option would be to rewrite them in C.
See the code for details what they do: https://github.com/legionus/kbd/blob/master/src/unicode_start#L18
There was a problem hiding this comment.
Other distros (Debian) ship these scripts as well. I would only feel comfortable disabling these by default, if we find out who actually needs them.
Another option would be to rewrite them in C.
See the code for details what they do: legionus/kbd@
master/src/unicode_start#L18
OK I see. I have an idea :) How about using multiple outputs? The bin output shouldn't be installed by default IIRC.
| }: | ||
|
|
||
| stdenv.mkDerivation rec { | ||
| strictDeps = true; |
There was a problem hiding this comment.
Nit: I find it very weird that this appears at the beginning (unfortunately we don't have official guidelines for that).
| { | ||
| updateScript = gitUpdater { | ||
| # No nicer place to find latest release. | ||
| url = "https://github.com/legionus/kbd.git"; | ||
| rev-prefix = "v"; | ||
| }; | ||
|
|
||
| tests = { | ||
| inherit (nixosTests) keymap kbd-setfont-decompress kbd-update-search-paths-patch; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Nit: I'd add these in 2 separate commits.
|
Mmh. There is more work to be done in |
|
Opened an alternative, slightly more minimal PR at: |
6 participants
This PR makes the parts of
kbdoptional that depend on bash:popencalls.unicode_startandunicode_stopare now optional. As far as I can tell, these are not used anyhow?Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.