-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
input-remapper: init at unstable-2022-02-09 (and add nixos module) #155290
Conversation
20c2f5b
to
738663b
Compare
Not sure if I put the module in the appropriate place. There's a FIXME there that needs investigating, I think it's this issue sezanzeb/input-remapper#140 |
738663b
to
e218cdc
Compare
Some tests are failing on aarch64, looks like they're sensitive to timing and run too slowly. Going to turn those off, don't want high load on a build system making tests flaky. |
d66896d
to
71ae5c3
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.
heya! welcome 👋
here's a few comments. nothing really bad, mostly just stylistic changes or things that could be done a little nicer. :)
, gtk3 | ||
, glib | ||
, gobject-introspection | ||
, xmodmap ? null # safe to override to null if you don't want xmodmap support |
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 a bit unusual for a couple reasons. the default argument will never be used (since the derivation only targets linux), but more importantly optional features usually have withX
boolean flags in the header. overriding with null
is a valid way of doing it, but an explicit flag is usually cleaner (especially if a dependency should be added to a feature in the future).
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.
How do I cleanly make xmodmap end up as null when withXmodmap is false?
let xmodmapOrNull = if withXmodmap then xmodmap else null; in
and then change all the references to xmodmap
to use xmodmapOrNull
?
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.
most derivations we've seen so far wouldn't do that, they'd put an ] ++ optional withXmodmap xmodmap ++ [
in place of xmodmap
in dependency lists. if there's more than one occurrence we'd do much the same as you suggested but with lists, ie
let
maybeXmodmap = optional withXmodmap xmodmap;
in ...
however your approach is also fine, it's just a little less future-proof if the input-remapper author decides to add additional dependencies for xmodmap support. :) if that's not likely there's nothing wrong with doing it like you have
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.
Added maybeXmodmap
, input_remapper_version ? "1.3.0" | ||
, input_remapper_src_rev ? "76c3cadcfaa3f244d62bd911aa6642a86233ffa0" | ||
, input_remapper_src_hash ? "sha256-llSgPwCLY34sAmDEuF/w2qeXdPFLLo1MIPBmbxwZZ3k=" |
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.
In addition to what pennae said, why are we not using the git tag instead of the revision?
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.
Since the app expects a specific hash set as COMMIT_HASH (see prePatch) we already need a specific rev, seemed duplicate info to set a rev and a tag?
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.
A tag is also a form of revision, using a literal one really only makes things more complicated in the derivation and for the end user.
There's also src.rev
which will always have the revision, set in the fetcher.
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.
If I reference src.rev that also won't pick up the rev from an overriden src.
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.
Tried using it and it's also always the input rev to the fetcher, not the commit hash that gets used, so it doesn't give the right output for COMMIT_HASH
further down.
If leaveDotGit
worked that'd be a satisfactory solution but we can't use it either due to hashes not being stable.
Leaving this as a draft for a bit, hopefully going to get some changes upstreamed and then can simplify this. |
47ea9a2
to
89945cd
Compare
Updated with upstream improvements, packaging is simpler now. Still waiting for #119942 to go in to change how the source rev is done. |
I wanted to use #119942 to make overriding the version work properly without having extra args to the package but it looks like that isn't going to get merged soon. I'd prefer to keep the Original forum discussion I took this idea from: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293/7?u=lun |
c1ab3a6
to
07c6db0
Compare
981d8f2
to
f9e919a
Compare
yeah, let's leave it with those extra parameters for now then. looks pretty good now we'd say, though we haven't tested anything. would also be nice to squash most of the commits down into three like the ones you started with :) |
Okay, I'll squash those tonight. |
Oh, also forgot to add release notes which the checklist says to do for a module addition. Will do that as well, might not be tonight though as I'm tired from work. |
f9e919a
to
6a4ff4f
Compare
6a4ff4f
to
3988079
Compare
3988079
to
91c7b73
Compare
/marvin opt-in |
Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here. |
Result of 2 packages blacklisted:
1 package built:
|
I think it looks good. We can always improve later. |
Motivation for this change
Add input-remapper package and module to allow remapping keys on most input devices.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes