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

input-remapper: init at unstable-2022-02-09 (and add nixos module) #155290

Merged
merged 4 commits into from
Feb 15, 2022

Conversation

LunNova
Copy link
Member

@LunNova LunNova commented Jan 17, 2022

Motivation for this change

Add input-remapper package and module to allow remapping keys on most input devices.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux (but emulated)
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@LunNova
Copy link
Member Author

LunNova commented Jan 17, 2022

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

@LunNova
Copy link
Member Author

LunNova commented Jan 17, 2022

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.

Copy link
Contributor

@pennae pennae left a 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. :)

pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
, gtk3
, glib
, gobject-introspection
, xmodmap ? null # safe to override to null if you don't want xmodmap support
Copy link
Contributor

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).

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added maybeXmodmap

pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
pkgs/tools/inputmethods/input-remapper/default.nix Outdated Show resolved Hide resolved
Comment on lines 21 to 23
, input_remapper_version ? "1.3.0"
, input_remapper_src_rev ? "76c3cadcfaa3f244d62bd911aa6642a86233ffa0"
, input_remapper_src_hash ? "sha256-llSgPwCLY34sAmDEuF/w2qeXdPFLLo1MIPBmbxwZZ3k="
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@LunNova
Copy link
Member Author

LunNova commented Jan 17, 2022

Leaving this as a draft for a bit, hopefully going to get some changes upstreamed and then can simplify this.

@LunNova LunNova changed the title input-remapper: init at 1.3.0 (and add nixos module) input-remapper: init at 1.4.1-dev (and add nixos module) Feb 7, 2022
@LunNova
Copy link
Member Author

LunNova commented Feb 7, 2022

Updated with upstream improvements, packaging is simpler now.

Still waiting for #119942 to go in to change how the source rev is done.

@LunNova
Copy link
Member Author

LunNova commented Feb 9, 2022

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 input_remapper_* args for now as other approaches don't allow for overriding easily.

Original forum discussion I took this idea from: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293/7?u=lun

@LunNova LunNova changed the title input-remapper: init at 1.4.1-dev (and add nixos module) input-remapper: init at 20220209 (and add nixos module) Feb 9, 2022
@LunNova LunNova marked this pull request as ready for review February 9, 2022 16:16
@LunNova LunNova changed the title input-remapper: init at 20220209 (and add nixos module) input-remapper: init at unstable-2022-02-09 (and add nixos module) Feb 9, 2022
@pennae
Copy link
Contributor

pennae commented Feb 9, 2022

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 :)

@LunNova
Copy link
Member Author

LunNova commented Feb 9, 2022

Okay, I'll squash those tonight.

@LunNova
Copy link
Member Author

LunNova commented Feb 10, 2022

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.

LunNova added a commit to LunNova/nixos-configs that referenced this pull request Feb 11, 2022
@LunNova
Copy link
Member Author

LunNova commented Feb 15, 2022

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Feb 15, 2022

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.

@kevincox
Copy link
Contributor

Result of nixpkgs-review pr 155290 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
1 package built:
  • input-remapper

@kevincox
Copy link
Contributor

I think it looks good. We can always improve later.

@kevincox kevincox merged commit 9ec2ae3 into NixOS:master Feb 15, 2022
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

5 participants