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

fprintd, libfprint : update to 1.90.1 #75435

Merged
merged 3 commits into from May 25, 2020
Merged

fprintd, libfprint : update to 1.90.1 #75435

merged 3 commits into from May 25, 2020

Conversation

ghost
Copy link

@ghost ghost commented Dec 10, 2019

Motivation for this change

Updates libfprint and fprintd. It also adds libpam-wrapper : a library made to test pam modules. This library is used by upstream to test the fprintd build.

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

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

The MR was merged, release still not tagged though.

pkgs/tools/security/fprintd/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/fprintd/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/fprintd/default.nix Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Dec 19, 2019

Hello @jtojnar : thank you for your review. Currently upstream does not compile against libfprint version 1.90 (with current derivation), I believe this might be related to the reason they did not tagged it yet.

@ghost
Copy link
Author

ghost commented Jan 19, 2020

We have two MR on upstream for releasing the 1.90.1 on both the library and the binary:

I've updated this PR with the latest versions.

@ghost
Copy link
Author

ghost commented Jan 31, 2020

I've pushed an update with the latest content of the merge requests on GitLab.
I'm having trouble to add libpam-wrapper (A helper to test PAM modules added in latest upstream commits) into the python context (this C library is also building a Python library with it).
If somebody knows how to handle that specific case :).

@ghost
Copy link
Author

ghost commented Feb 10, 2020

1.90.1 version has been released on both projects 3 hours ago, I'm currently working on integrating it. I will submit this PR asap. Is it too late for the feature freeze of 20.03 @worldofpeace ?

@ghost ghost marked this pull request as ready for review February 10, 2020 21:00
@ghost
Copy link
Author

ghost commented Feb 10, 2020

Finished to port 1.90.1 version :

  • libfprint : Version 1.90.1
  • libpam-wrapper : New dependency for building/testing the software
  • fprintd : Version 1.90.1

Current testing :

  • Worked well for enrolling and using it with on my Lenovo Thinkpad T495 (06cb:00bd reader).

The 1.90.x adds native support for a lot of devices that are already on the market, and I believe it could be good thing to have it in 20.03.

@ghost ghost changed the title fprintd: 1.0 -> 1.90 fprintd, libfprint : update to 1.90.1 Feb 10, 2020
@worldofpeace
Copy link
Contributor

We can only backport bugfixes releases, I'm not sure this qualifies as one?

@jtojnar
Copy link
Contributor

jtojnar commented Feb 10, 2020

Given that this is still a pre-release, I would move this to 20.09

@ghost
Copy link
Author

ghost commented Feb 10, 2020

@jtojnar : I don't see any mention that this one is deemed a pre-release ? They seems to have wanted to release a 1.90.1 version to support the devices while iterating towards a 2.0 version (based on comment on this MR on libfprint)

But let's keep it for 20.09 anyway ;)

@ghost
Copy link
Author

ghost commented Feb 10, 2020

Bonus question : should I add myself to the maintainers ?

@worldofpeace
Copy link
Contributor

Bonus question : should I add myself to the maintainers ?

Totally

@nixos-discourse
Copy link

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/3

@LEXUGE
Copy link
Contributor

LEXUGE commented Feb 25, 2020

@elyhaka
On my X1 Carbon 7th Gen (20R1), a patch is needed for it to enroll. I successfully resolved yesterday's problem by using patch

@jtojnar
Copy link
Contributor

jtojnar commented Feb 25, 2020

@LEXUGE Is there an upstream bug report/merge request for that?

@LEXUGE
Copy link
Contributor

LEXUGE commented Feb 25, 2020

@LEXUGE Is there an upstream bug report/merge request for that?

Not really but it should be a bug

@jtojnar
Copy link
Contributor

jtojnar commented Feb 25, 2020

Could you open it then please? Applying random patches for unreported bugs rarely leads to upstream fixes.

@LEXUGE
Copy link
Contributor

LEXUGE commented Feb 25, 2020

Could you open it then please? Applying random patches for unreported bugs rarely leads to upstream fixes.

I will do it later.

@LEXUGE
Copy link
Contributor

LEXUGE commented Feb 29, 2020

@ghost
Copy link
Author

ghost commented Feb 29, 2020

@LEXUGE : While being an issue on upstream, the way I pass the path to the derivation seems to be broken as is. I'm not sure why it works on my side, probably because I have a state directory that is populated by many tries of many versions while working on the derivation.

@ghost
Copy link
Author

ghost commented May 17, 2020

@jtojnar Thank you very much for your time and your extensive review, let me know if there is other parts I can improve.

@ghost
Copy link
Author

ghost commented May 18, 2020

I've redone a round of tests (enrolling, login with different managers like GDM etc...) it works correctly on my side.

@jtojnar
Copy link
Contributor

jtojnar commented May 18, 2020

I tried the following but gcc -print-file-name=libpam_wrapper.so does not seem to be able to find the libpam_wrapper.so, not sure how to fix that:

diff --git a/pkgs/tools/security/fprintd/default.nix b/pkgs/tools/security/fprintd/default.nix
index f589ffe8c76..0f5b2ac2244 100644
--- a/pkgs/tools/security/fprintd/default.nix
+++ b/pkgs/tools/security/fprintd/default.nix
@@ -20,7 +20,6 @@
 , systemd
 , libfprint
 , python3
-, pypamtest
 }:
 
 stdenv.mkDerivation rec {
@@ -43,6 +42,20 @@ stdenv.mkDerivation rec {
       url = "https://gitlab.freedesktop.org/libfprint/fprintd/-/merge_requests/50.patch";
       sha256 = "1wqkk7bqig0fhjbq1487kxkh7yc9356qjsac1x27pzgakbayxpyy";
     })
+    # Fixes issue with ":" when there is multiple paths (might be the case on NixOS)
+    # https://gitlab.freedesktop.org/libfprint/fprintd/-/merge_requests/50
+    (fetchpatch {
+      url = "https://gitlab.freedesktop.org/libfprint/fprintd/-/commit/d7fec03f24d10f88d34581c72f0eef201f5eafac.patch";
+      sha256 = "QNN05WF4YZ0XiTwm5NkfqZDuQpyXlnrh+RJF9SNsCDk=";
+    })
+    (fetchpatch {
+      url = "https://gitlab.freedesktop.org/libfprint/fprintd/-/commit/4f3589c0dc0feb67c45cdae7e692d70fe493f3d8.patch";
+      sha256 = "2kYo8uqMja3r9SroKB5cR777MJ/BGig4PB1qwLUQ01U=";
+    })
+    (fetchpatch {
+      url = "https://gitlab.freedesktop.org/libfprint/fprintd/-/commit/f401f399a85dbeb2de165b9b9162eb552ab6eea7.patch";
+      sha256 = "Pga+/QEkln8DOwGZfKM1r2urJX4Y3X0bozWWxKZ5ia0=";
+    })
   ];
 
   nativeBuildInputs = [
@@ -91,7 +104,10 @@ stdenv.mkDerivation rec {
   # Ideally, we should find a way to execute the test here
   # instead of the buildPhase.
   doCheck = true;
-  checkPhase = ":";
+
+  postPatch = ''
+    patchShebangs po/check-translations.sh
+  '';
 
   meta = with stdenv.lib; {
     homepage = "https://fprint.freedesktop.org/";

@ghost
Copy link
Author

ghost commented May 19, 2020

@jtojnar IIRC, GCC checks the LIBRARY_PATH folder. I don't know how acceptable is this solution, but adding:

LIBRARY_PATH = stdenv.lib.makeLibraryPath [ libpam-wrapper ];

Fixes this issue.

@jtojnar
Copy link
Contributor

jtojnar commented May 19, 2020

I think it is okay for tests in preCheck, it should not affect things much. But I wonder why they do not use compiler.find_library(). Edit: Right, does not have a way to return the path: mesonbuild/meson#6880

@ghost
Copy link
Author

ghost commented May 19, 2020

I'm now fighting with a python program not loaded correctly from meson:

tests/meson.build:50:0: ERROR: Program(s) ['unittest_inspector.py'] not found or not executable

I'm not sure how to fix this one. (I'm not familiar enough with meson).

@jtojnar
Copy link
Contributor

jtojnar commented May 19, 2020

Missed two commits from https://gitlab.freedesktop.org/libfprint/fprintd/-/merge_requests/40, now picking it completely:

diff --git a/pkgs/tools/security/fprintd/default.nix b/pkgs/tools/security/fprintd/default.nix
index 3f25e813279..f5bb97a65a5 100644
--- a/pkgs/tools/security/fprintd/default.nix
+++ b/pkgs/tools/security/fprintd/default.nix
@@ -42,6 +42,20 @@ stdenv.mkDerivation rec {
       url = "https://gitlab.freedesktop.org/libfprint/fprintd/-/merge_requests/50.patch";
       sha256 = "1wqkk7bqig0fhjbq1487kxkh7yc9356qjsac1x27pzgakbayxpyy";
     })
+    # Fixes issue with ":" when there is multiple paths (might be the case on NixOS)
+    # https://gitlab.freedesktop.org/libfprint/fprintd/-/merge_requests/50
+    (fetchpatch {
+      url = "https://gitlab.freedesktop.org/libfprint/fprintd/-/commit/d7fec03f24d10f88d34581c72f0eef201f5eafac.patch";
+      sha256 = "QNN05WF4YZ0XiTwm5NkfqZDuQpyXlnrh+RJF9SNsCDk=";
+    })
+    (fetchpatch {
+      url = "https://gitlab.freedesktop.org/libfprint/fprintd/-/merge_requests/40.patch";
+      sha256 = "43uPihK6HhygHw1Qplwci80Wseq/S77VUp+OdEECHmM=";
+    })
+    (fetchpatch {
+      url = "https://gitlab.freedesktop.org/libfprint/fprintd/-/commit/f401f399a85dbeb2de165b9b9162eb552ab6eea7.patch";
+      sha256 = "Pga+/QEkln8DOwGZfKM1r2urJX4Y3X0bozWWxKZ5ia0=";
+    })
   ];
 
   nativeBuildInputs = [
@@ -75,10 +89,6 @@ stdenv.mkDerivation rec {
     pypamtest
   ];
 
-  PKG_CONFIG_DBUS_1_INTERFACES_DIR = "${placeholder "out"}/share/dbus-1/interfaces";
-  PKG_CONFIG_POLKIT_GOBJECT_1_POLICYDIR = "${placeholder "out"}/share/polkit-1/actions";
-  PKG_CONFIG_DBUS_1_DATADIR = "${placeholder "out"}/share";
-
   mesonFlags = [
     "-Dgtk_doc=true"
     "-Dpam_modules_dir=${placeholder "out"}/lib/security"
@@ -87,10 +97,18 @@ stdenv.mkDerivation rec {
     "-Dsystemd_system_unit_dir=${placeholder "out"}/lib/systemd/system"
   ];
 
-  # Ideally, we should find a way to execute the test here
-  # instead of the buildPhase.
+  PKG_CONFIG_DBUS_1_INTERFACES_DIR = "${placeholder "out"}/share/dbus-1/interfaces";
+  PKG_CONFIG_POLKIT_GOBJECT_1_POLICYDIR = "${placeholder "out"}/share/polkit-1/actions";
+  PKG_CONFIG_DBUS_1_DATADIR = "${placeholder "out"}/share";
+
+  # FIXME: Ugly hack for tests to find libpam_wrapper.so
+  LIBRARY_PATH = stdenv.lib.makeLibraryPath [ python3.pkgs.pypamtest ];
+
   doCheck = true;
-  checkPhase = ":";
+
+  postPatch = ''
+    patchShebangs po/check-translations.sh
+  '';
 
   meta = with stdenv.lib; {
     homepage = "https://fprint.freedesktop.org/";

But it still fails and the errors are even less helpful:

21/26 fprintd:PAM+test_pam_fprintd+TestPamFprintd / TestPamFprintd.test_pam_fprintd_auth                                 FAIL           0.97s (exit status 1)

--- command ---
08:42:36 LD_PRELOAD='/nix/store/7a2q4pykjq9c0jjmdf2v96wjpsvzy8i1-libpam-wrapper-1.1.3/lib/libpam_wrapper.so' TOPSRCDIR='/build/source' TOPBUILDDIR='/build/source/build' G_SLICE='always-malloc' MALLOC_CHECK_='2' G_DEBUG='fatal-warnings' PAM_WRAPPER_SERVICE_DIR='/build/source/build/tests/pam/services' PAM_WRAPPER_DEBUGLEVEL='2' PAM_WRAPPER='1' MALLOC_PERTURB_='55' /nix/store/2dcsn57cgaxs92ha5swihrab0g3l2h6g-python3-3.7.7/bin/python3 /build/source/tests/pam/test_pam_fprintd.py TestPamFprintd.test_pam_fprintd_auth
--- stdout ---
Using template from /build/source/tests/dbusmock/fprintd.py
test_pam_fprintd_auth (__main__.TestPamFprintd) ... ERROR

======================================================================
ERROR: test_pam_fprintd_auth (__main__.TestPamFprintd)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/source/tests/pam/test_pam_fprintd.py", line 99, in test_pam_fprintd_auth
    res = pypamtest.run_pamtest("toto", "fprintd-pam-test", [tc], [ 'unused' ])
pypamtest.PamTestError: [Errno 1] pam_start failed()

----------------------------------------------------------------------
Ran 1 test in 0.649s

FAILED (errors=1)
--- stderr ---
PWRAP_DEBUG[<unknown> (965)] - pwrap_init: Initialize pam_wrapper
PWRAP_DEBUG[<unknown> (965)] - pwrap_init: Copy /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0 to /tmp/pam.g/lib/libpam.so.0
PWRAP_DEBUG[<unknown> (965)] - copy_confdir: Copy config files from /build/source/build/tests/pam/services to /tmp/pam.g
PWRAP_DEBUG[<unknown> (965)] - pwrap_init: Successfully initialized pam_wrapper
PWRAP_DEBUG[<unknown> (967)] - pwrap_init: Initialize pam_wrapper
PWRAP_DEBUG[<unknown> (967)] - pwrap_init: Copy /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0 to /tmp/pam.i/lib/libpam.so.0
PWRAP_DEBUG[<unknown> (967)] - copy_confdir: Copy config files from /build/source/build/tests/pam/services to /tmp/pam.i
PWRAP_DEBUG[<unknown> (967)] - pwrap_init: Successfully initialized pam_wrapper
PWRAP_DEBUG[<unknown> (970)] - pwrap_init: Initialize pam_wrapper
PWRAP_DEBUG[<unknown> (970)] - pwrap_init: Copy /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0 to /tmp/pam.l/lib/libpam.so.0
PWRAP_DEBUG[<unknown> (970)] - copy_confdir: Copy config files from /build/source/build/tests/pam/services to /tmp/pam.l
PWRAP_DEBUG[<unknown> (970)] - pwrap_init: Successfully initialized pam_wrapper
PWRAP_DEBUG[<unknown> (965)] - pwrap_load_lib_handle: Opened /tmp/pam.g/lib/libpam.so.0

-------

@ghost
Copy link
Author

ghost commented May 19, 2020

I haven't been able to fix the tests, the reason for the pam_start failure is not logged anywhere, I could not manage to find it. I also tried to change libpam-wrapper version to see if the behavior changed (tried the version on Debian and ArchLinux) without any luck.

@jtojnar
Copy link
Contributor

jtojnar commented May 19, 2020

Tried running it with

    substituteInPlace tests/pam/meson.build --replace PAM_WRAPPER_DEBUGLEVEL=2 PAM_WRAPPER_DEBUGLEVEL=3

in postPatch does not lead to much more info:

PWRAP_DEBUG[<unknown> (987)] - pwrap_init: Initialize pam_wrapper
PWRAP_TRACE[<unknown> (987)] - pwrap_init: pam_wrapper config dir: /tmp/pam.7
PWRAP_TRACE[<unknown> (987)] - pwrap_init: PAM path: /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so
PWRAP_TRACE[<unknown> (987)] - pwrap_init: PAM library: libpam.so.2.0.0
PWRAP_TRACE[<unknown> (987)] - pwrap_init: Reconstructed PAM path: /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0
PWRAP_DEBUG[<unknown> (987)] - pwrap_init: Copy /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0 to /tmp/pam.7/lib/libpam.so.0
PWRAP_TRACE[<unknown> (987)] - pwrap_init: Using libpam path: /tmp/pam.7/lib/libpam.so.0
PWRAP_DEBUG[<unknown> (987)] - copy_confdir: Copy config files from /build/source/build/tests/pam/services to /tmp/pam.7
PWRAP_TRACE[<unknown> (987)] - copy_ftw: Copying /build/source/build/tests/pam/services/fprintd-pam-test
PWRAP_DEBUG[<unknown> (987)] - pwrap_init: Successfully initialized pam_wrapper
PWRAP_DEBUG[<unknown> (1086)] - pwrap_init: Initialize pam_wrapper
PWRAP_TRACE[<unknown> (1086)] - pwrap_init: pam_wrapper config dir: /tmp/pam.T
PWRAP_TRACE[<unknown> (1086)] - pwrap_init: PAM path: /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so
PWRAP_TRACE[<unknown> (1086)] - pwrap_init: PAM library: libpam.so.2.0.0
PWRAP_TRACE[<unknown> (1086)] - pwrap_init: Reconstructed PAM path: /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0
PWRAP_DEBUG[<unknown> (1086)] - pwrap_init: Copy /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0 to /tmp/pam.T/lib/libpam.so.0
PWRAP_TRACE[<unknown> (1086)] - pwrap_init: Using libpam path: /tmp/pam.T/lib/libpam.so.0
PWRAP_DEBUG[<unknown> (1086)] - copy_confdir: Copy config files from /build/source/build/tests/pam/services to /tmp/pam.T
PWRAP_TRACE[<unknown> (1086)] - copy_ftw: Copying /build/source/build/tests/pam/services/fprintd-pam-test
PWRAP_DEBUG[<unknown> (1086)] - pwrap_init: Successfully initialized pam_wrapper
PWRAP_DEBUG[<unknown> (1149)] - pwrap_init: Initialize pam_wrapper
PWRAP_TRACE[<unknown> (1149)] - pwrap_init: pam_wrapper config dir: /tmp/pam.M
PWRAP_TRACE[<unknown> (1149)] - pwrap_init: PAM path: /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so
PWRAP_TRACE[<unknown> (1149)] - pwrap_init: PAM library: libpam.so.2.0.0
PWRAP_TRACE[<unknown> (1149)] - pwrap_init: Reconstructed PAM path: /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0
PWRAP_DEBUG[<unknown> (1149)] - pwrap_init: Copy /nix/store/cjwsxn95qzjbhxdilbb3g87xqdx66vvq-openpam-20170430/lib/libpam.so.2.0.0 to /tmp/pam.M/lib/libpam.so.0
PWRAP_TRACE[<unknown> (1149)] - pwrap_init: Using libpam path: /tmp/pam.M/lib/libpam.so.0
PWRAP_DEBUG[<unknown> (1149)] - copy_confdir: Copy config files from /build/source/build/tests/pam/services to /tmp/pam.M
PWRAP_TRACE[<unknown> (1149)] - copy_ftw: Copying /build/source/build/tests/pam/services/fprintd-pam-test
PWRAP_DEBUG[<unknown> (1149)] - pwrap_init: Successfully initialized pam_wrapper
PWRAP_TRACE[<unknown> (987)] - pwrap_pam_start: pam_start service=fprintd-pam-test, user=toto
PWRAP_DEBUG[<unknown> (987)] - pwrap_load_lib_handle: Opened /tmp/pam.7/lib/libpam.so.0

PWRAP_TRACE[<unknown> (987)] - pwrap_pam_start: pam_start rc=4
PWRAP_TRACE[<unknown> (987)] - pwrap_destructor: entering pwrap_destructor
PWRAP_TRACE[<unknown> (987)] - pwrap_destructor: destructor called for pam_wrapper dir /tmp/pam.7
PWRAP_TRACE[<unknown> (987)] - p_rmdirs_at: p_rmdirs_at removing /tmp/pam.7 at CWD

PWRAP_TRACE[<unknown> (987)] - p_rmdirs_at: p_rmdirs_at removing lib at fd=3

only that it must be failing somewhere in libpam_pam_start.

Replacing openpam by linux-pam seems to fix it, though.

@ghost
Copy link
Author

ghost commented May 19, 2020

I should switch the dependency from openpam to linux-pam in libpam-wrapper then ? I thought they were interchangeable seamlessly, it seems not.

@ghost
Copy link
Author

ghost commented May 19, 2020

@jtojnar I've pushed an update with the switch from openpam to linux-pam. linux-pam seems to be the de-facto standard for linux distributions, while openpam seems to be more related to the *BSD distributions, and is generally provided as an alternative PAM implementation on Linux distributions.

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for all your hard work 🦸‍♀️

@ghost
Copy link
Author

ghost commented May 19, 2020

Thanks a lot for your help, time and reviews!

@ghost
Copy link
Author

ghost commented May 25, 2020

Is anything still preventing this PR from being merged ? :)

@jtojnar jtojnar merged commit 0af23b0 into NixOS:master May 25, 2020
@jtojnar
Copy link
Contributor

jtojnar commented May 25, 2020

Should be fine, thanks again.

@ghost
Copy link
Author

ghost commented May 25, 2020

Thank you!

@ghost ghost deleted the fprintd branch May 25, 2020 10:25
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/t495-graphics-glitched/7950/6

@veprbl
Copy link
Member

veprbl commented Nov 14, 2020

See regression reported in #103739

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

6 participants