-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
swaylock-fprintd: init at unstable-20230130 #226519
Conversation
193e7f8
to
6294593
Compare
6294593
to
84dfb46
Compare
Nice, this looks almost exactly the same at the one I've been using (I just never bothered PR'ing it) If no one else does, I'll try to give this a review within a few days and may have comments about whether it should be combined with the existing swaylock package or not |
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.
I gave it a quick review but it'd be great if someone else could handle this.
IMO it's also not ideal that there's no stable release yet and I wonder how actively it'll get updated / for how long it'll get maintained.
It might also make sense to override the swaylock package instead of duplicating the code but I'm fine with it either way (IMO both variants have advantages and drawbacks).
e5f4cee
to
7d5036c
Compare
I've updated the PR regarding the feedback based on swaylock-derivation, @lilyinstarlight version and mine :) |
7d5036c
to
7b301d1
Compare
Hey @lilyinstarlight, do you have time to review or should I ask for merge as good-as-is for now? ✌️ |
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 looks pretty good to me!
I think my only reservation with this is that it's still not clear how long the fork will actually be maintained (they kept it updated for a bit, but haven't rebased their patches on the newer more invasive swaylock changes over the last two months)
I'm not opposed to just merging this to nixpkgs and removing it later if the fork does become genuinely abandoned, though
Also given it may lag a bit, I think having this as a separate derivation instead of overriding the swaylock one (e.g. to add a fetchpatch
that pulls down the PR diff) is probably for the best given the upstream PR has already accumulated merge conflicts
|
||
stdenv.mkDerivation rec { | ||
pname = "swaylock-fprintd"; | ||
version = "unstable-20230130"; |
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.
version = "unstable-20230130"; | |
version = "unstable-2023-01-30"; |
The format documented in the manual for unstable git versions is unstable-YYYY-MM-DD
(this may have been my bad in the derivation I wrote...)
glib | ||
meson | ||
ninja | ||
pkg-config |
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.
pkg-config |
I just noticed I had pkg-config
in my derivation twice for some reason...
swayosd = callPackage ../applications/window-managers/sway/osd.nix { }; | ||
swaylock-fprintd = callPackage ../applications/window-managers/sway/lock-fprintd.nix { }; |
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.
swayosd = callPackage ../applications/window-managers/sway/osd.nix { }; | |
swaylock-fprintd = callPackage ../applications/window-managers/sway/lock-fprintd.nix { }; | |
swaylock-fprintd = callPackage ../applications/window-managers/sway/lock-fprintd.nix { }; | |
swayosd = callPackage ../applications/window-managers/sway/osd.nix { }; |
Sort
@SebTM I realize this PR was left un-merged for a long time, but aside from that, is there a reason why this was closed? Have circumstances changed that make it unnecessary? I stumbled upon this when trying to figure out how I might go about enabling fingerprint unlock myself... |
Hey, after we had swaywm/swaylock#283 I was using it quite some time but it seems like https://github.com/SL-RU/swaylock-fprintd does not really get maintained/updated. We revived the feedback from a swaylock-maintainer (swaywm/swaylock#283 (comment)) that even though I disagree as the feature-integration is very lose and would allow build without the feature/removing it in case of nobody will help with maintaining/issues (as there are plenty of people requesting/following along) to not wanting to maintain this so not merge it. We came along with an alternative solution to make swaylock extensible by adding SIGUSR2 to indicate failed unlock and use the existing SIGUSR1 which unlocks the session for the case our external program records successful authentication. I gave it a shot here and @lilyinstarlight followed up with a better (my C-skills are very rudimentary) implementation, which sadly got rejected again by the maintainer: swaywm/swaylock#324 (comment) Now there is swaylock-effects which we build from https://github.com/jirutka/swaylock-effects after https://github.com/mortie/swaylock-effects got unmaintained and has an open PR again jirutka/swaylock-effects#49 which seems to be considered. But this is also not optimal (from my POV) as the latest version there is also already building against an older swaylock release from a commit shortly before swaylock-v1.7.1 release. From my personal result I have ditched sway entirely due to this and other issues and switched to gnome-wayland but considering Hyprland (with hyprlock, or if I get it working gdm or greetd which I use for login if I can get it working as lockscreen somehow - not looked into it so far) |
Just curious, this was my doing before this PR existed. # The `build.meson` does not use `pkg-config` for DBus interfaces,
# therefor `PKG_CONFIG_DBUS_1_INTERFACES_DIR` does not apply.
# <https://github.com/jirutka/swaylock-effects/pull/49#issuecomment-1932220922>
postPatch = let
dbusInterfacesDir = (pkgs.symlinkJoin {
name = "${self.pname}-${self.version}_dbus-interfaces-dir";
paths = self.buildInputs;
pathsToLink = [ "share/dbus-1/interfaces" ];
}) + "/share/dbus-1/interfaces";
in super.postPatch or "" + ''
sed -i 's@/usr/share/dbus-1/interfaces@${dbusInterfacesDir}@g' \
fingerprint/meson.build
''; This pattern ought to work for any package that uses any number of interfaces from any number of |
Description of changes
For now init the fork with proper fprintd-dbus support but I haven't fully given up for a potential merge: swaywm/swaylock#283 (comment)
I could also make it more generic and combine it with swaylock but I was unsure about this bigger refactoring so feel free to discuss ✌🏻
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/
)