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

libfprint: 1.0 -> 1.90 #74656

Merged
merged 1 commit into from Dec 1, 2019
Merged

libfprint: 1.0 -> 1.90 #74656

merged 1 commit into from Dec 1, 2019

Conversation

@Elyhaka
Copy link
Contributor

Elyhaka commented Nov 29, 2019

Motivation for this change

A huge rewrite of libfprint (fingerprint reader library) has been merged and released recently. It adds supports for USB connected fingerprint readers. (Which will add support for latest ThinkPad laptops).

I've splitted the thinkpad fork package from the main one because the dependencies and version numbers are not matching anymore. If that's an issue I can remerge them together.

DISCLAIMER : I haven't been able to test the package update yet, Lenovo has removed the firmware upgrade related to the model I'm using right now (it was causing issues, they are going to make it available again in the next days), and since this package is hardware dependant for testing, if someone with updated hardware (or previously working hardware) can test it, it would be great.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @abbradar

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Nov 29, 2019

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/working-on-updating-libfprint-to-1-90/4958/1

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Nov 29, 2019

@GrahamcOfBorg build libfprint

@cizra

This comment has been minimized.

Copy link
Contributor

cizra commented Nov 29, 2019

@Elyhaka

This comment has been minimized.

Copy link
Contributor Author

Elyhaka commented Nov 30, 2019

Hello @cizra, thank you for the feedback. Do you have any idea of who should be put as a maintainer of the two packages ?

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Nov 30, 2019

Is the fork still required, should we remove libfprint-thinkpad? @abbradar could still stay as a maintainer for libfprint itself.

@Elyhaka

This comment has been minimized.

Copy link
Contributor Author

Elyhaka commented Nov 30, 2019

@Mic92 : I believe the fork is not required since it is available only for some edge-case hardware. Furthermore, this fork seems to require that the fingerprint reader has been initialized with a Windows setup. The main issue is that some people may already be using it, and it will simply drop the support for them : which is not what they would expect. On other distros this would have been put in a user repositories (like ArchLinux's AUR).

I do not know what are the guidelines for a package to be added or removed to/from NixOS repositories. Is there any documentation I can refer to ?

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Nov 30, 2019

We usually add removed packages to pkgs/top-level/aliases.nix to inform users.

@Elyhaka

This comment has been minimized.

Copy link
Contributor Author

Elyhaka commented Nov 30, 2019

Okay, let me know if I should remove it.

@Elyhaka

This comment has been minimized.

Copy link
Contributor Author

Elyhaka commented Nov 30, 2019

@Mic92 I see that you're mainting NUR. Do you think libfprint-thinkpad would be a better fit there ?

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Dec 1, 2019

@Elyhaka probably. It would be just up to someone to put it there.

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Dec 1, 2019

@Elyhaka I would just drop it for now.

@Elyhaka

This comment has been minimized.

Copy link
Contributor Author

Elyhaka commented Dec 1, 2019

@Mic92 : I've removed it, and added the pkgs/top-level/aliases.nix for both fprintd-thinkpad and libfprint-thinkpad.

@ofborg ofborg bot added the 8.has: clean-up label Dec 1, 2019
@Mic92 Mic92 merged commit bfed052 into NixOS:master Dec 1, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Dec 15, 2019

This likely broke fprintd: https://hydra.nixos.org/build/108073131/nixlog/1

It is trying to find libpfrint.pc but 1.90 provides libpfrint2.pc.

@c0bw3b

This comment has been minimized.

Copy link
Contributor

c0bw3b commented Dec 15, 2019

Upstream advertises 1.90 as a sort of beta for 2.x

@Elyhaka

This comment has been minimized.

Copy link
Contributor Author

Elyhaka commented Dec 16, 2019

Hello, I have just tested locally, and it broke fprintd : I thought I checked it correctly, but it seems I missed this. I was working directly with the library itself most of the time recently so I didn't noticed it. I would like to apologize for that, I'll be more careful next time before proposing a PR.

I have a draft PR #75435 waiting for upstream to merge the update on fprintd : maybe we should revert this change, and I add both updates inside the other draft PR ?

Upstream states that :

The current codebase is considered stable at this point. However, due to the lack of wider testing it is only released as a 1.90.0 release which can be considered a beta-release for 2.0.

I had understanding this as "It works now, but we will wait a bit before we release 2.0" (mostly for firmware upgrades from what I've read in the issues/MR on upstream).

@gnidorah

This comment has been minimized.

Copy link
Member

gnidorah commented Dec 17, 2019

@Elyhaka

so I didn't noticed it

Using nix-review command should help you not to miss such issues: https://github.com/Mic92/nixpkgs-review
For e.g. nix-review pr 74656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.