nextcloud-client/owncloud-client/qtkeychain: Fix GNOME keyring integration #71874
Conversation
Because of a small mistake in the use of lib.lists.optional [^1], libsecret was not being used during the build of qtkeychain. This commit fixes that mistake. But since including libsecret apparently breaks some packages, this commit makes the inclusion of libsecret optional via the withLibsecret option, which defaults to false. Therefore, this change should be free of any consequences. Note that this commit also changes one occurrence of lib.lists.optional that is used for Darwin-specific buildInputs. I have tested (with nix-review) whether this breaks anything on macOS. Everything seems to be fine. The output of nix-review has been: “Nothing changed." [^1]: lib.lists.optional wraps its input in a list. Therefore, the argument to this function should usually not be a list because then the result is a nested list, which is, in most cases, not the desired output. lib.lists.optionals — note the ’s’ at the end — on the other hand does not wrap its input in a list.
This change enables integration with the GNOME keyring. See commit c691321 [^1] and the current derivation of nextcloud-client [^2] for comparison. [^1]: c691321 [^2]: https://github.com/NixOS/nixpkgs/blob/8825940cec53cdd300186110acb6a51895ecd4f9/pkgs/applications/networking/nextcloud-client/default.nix
nextcloud-client now *actually* uses libsecret (via qtkeychain), which enables integration with the GNOME keyring for real this time.
|
Should be unrelated but seems like the client has trouble blitting its window, |
|
As discussed n one of the previous PRs, we should build with libsecret support by default, unconditionally. |
I doubt that has anything to do with my pull request. Could you please check whether you also have this problem on the master branch?
Enabling libsecret by default breaks the following packages: matrique/spectral-unstable, psi-plus, quaternion, trojita. I did not want to introduce a breaking change, and I did not want to touch unrelated packages. That's why I did it this way. I see two alternatives to my solution:
With alternative 1, one could change the breaking packages to withLibsecret = false. I could do that, if you want. Alternative 2 means breaking these four packages. Someone would have to fix them. How do we proceed? PS: This is my first contribution to NixOS. |
|
Could you share the failure logs? Are they really caused by this? diff --git a/pkgs/development/libraries/qtkeychain/default.nix b/pkgs/development/libraries/qtkeychain/default.nix
index 3de84d85911..31b944e6611 100644
--- a/pkgs/development/libraries/qtkeychain/default.nix
+++ b/pkgs/development/libraries/qtkeychain/default.nix
@@ -21,7 +21,10 @@
patches = if withQt5 then null else [ ./0001-Fixes-build-with-Qt4.patch ];
- cmakeFlags = [ "-DQT_TRANSLATIONS_DIR=share/qt/translations" ];
+ cmakeFlags = [
+ "-DQT_TRANSLATIONS_DIR=share/qt/translations"
+ "-DLIBSECRET_SUPPORT=ON"
+ ];
nativeBuildInputs = [ cmake ]
++ stdenv.lib.optional (!stdenv.isDarwin) [ pkgconfig ] # for finding libsecretseems to be a no-op, so I do not see how it could break anything. |
|
The lines that you quoted only are there so that no error messages are shown if libsecret isn't found. The really relevant change is this: - buildInputs = stdenv.lib.optional (!stdenv.isDarwin) [ libsecret ]
+ buildInputs = stdenv.lib.optionals useLibsecret [ libsecret ]Note the use of stdenv.lib.optionals instead of stdenv.lib.optional. The same effect could have been achieved by the following change: - buildInputs = stdenv.lib.optional (!stdenv.isDarwin) [ libsecret ]
+ buildInputs = stdenv.lib.optional useLibsecret libsecretTo quote from my commit message:
libsecret had never actually been used. When it is used by default, some package builds break. psi-plus, for example: |
You are right, I was missing this. We should really have something checking for this.
I believe we built it with the support for libsecret (if it were not found, it would fail) but, since it was not linked against, we had to manually override the linking. Now that it is linked against it, we should be able to drop the
It is caused by this change: --- /nix/store/bkfwmwzl9iyfmq6r5kx3sk3pin1h4rys-qtkeychain-qt5-0.9.1/lib/cmake/Qt5Keychain/Qt5KeychainLibraryDepends-release.cmake
+++ /nix/store/nh2qv2m9bmbf6jyb7753zhk6nlx62da1-qtkeychain-qt5-0.9.1/lib/cmake/Qt5Keychain/Qt5KeychainLibraryDepends-release.cmake
@@ -8,9 +8,9 @@
# Import target "qt5keychain" for configuration "Release"
set_property(TARGET qt5keychain APPEND PROPERTY IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(qt5keychain PROPERTIES
- IMPORTED_LINK_INTERFACE_LIBRARIES_RELEASE "Qt5::Core;Qt5::DBus"
- IMPORTED_LOCATION_RELEASE "/nix/store/bkfwmwzl9iyfmq6r5kx3sk3pin1h4rys-qtkeychain-qt5-0.9.1/lib/libqt5keychain.so.0.9.1"
+ IMPORTED_LINK_INTERFACE_LIBRARIES_RELEASE "Qt5::Core;secret-1;gio-2.0;gobject-2.0;glib-2.0;Qt5::DBus"
+ IMPORTED_LOCATION_RELEASE "/nix/store/nh2qv2m9bmbf6jyb7753zhk6nlx62da1-qtkeychain-qt5-0.9.1/lib/libqt5keychain.so.0.9.1"
IMPORTED_SONAME_RELEASE "libqt5keychain.so.1"
)
list(APPEND _IMPORT_CHECK_TARGETS qt5keychain )I would say it is because the upstream incorrectly marks the library dependencies. frankosterfeld/qtkeychain#153 should fix that: --- a/pkgs/development/libraries/qtkeychain/default.nix
+++ b/pkgs/development/libraries/qtkeychain/default.nix
@@ -1,4 +1,7 @@
-{ stdenv, fetchFromGitHub, cmake, pkgconfig, qt4 ? null
+{ stdenv
+, fetchFromGitHub
+, fetchpatch
+, cmake, pkgconfig, qt4 ? null
, withQt5 ? false, qtbase ? null, qttools ? null
, darwin ? null
, libsecret
@@ -19,7 +22,16 @@
sha256 = "0h4wgngn2yl35hapbjs24amkjfbzsvnna4ixfhn87snjnq5lmjbc"; # v0.9.1
};
- patches = if withQt5 then null else [ ./0001-Fixes-build-with-Qt4.patch ];
+ patches = [
+ # Do not propagate internal dependency on libsecret.
+ # https://github.com/frankosterfeld/qtkeychain/pull/153
+ (fetchpatch {
+ url = "https://github.com/frankosterfeld/qtkeychain/commit/94d719168f874c07a1edfb1aca7e21875063b292.patch";
+ sha256 = "1mz72xalmsy336xijazlh8ffzsaqag2pch6mg8pgxiq7xy31fvxk";
+ })
+ ] ++ stdenv.lib.optionals (!withQt5) [
+ ./0001-Fixes-build-with-Qt4.patch
+ ];
cmakeFlags = [ "-DQT_TRANSLATIONS_DIR=share/qt/translations" ];
|
|
Hi. Is there any news on this PR? |
Yeah. I just checked with the current master. The Nextcloud and Owncloud clients work fine now. This has been fixed with commit 839c0ea. |
Janik-Haag
added
the
12. first-time contribution
Motivation for this change
There had been a small mistake in the package definition of qtkeychain. The result had been that qtkeychain was being compiled without libsecret support. Therefore, integration with the GNOME keyring had not been working, forcing users to put in credentials whenever they started the clients for Nextcloud or Owncloud.
This pull request takes care that qtkeychain can be built with libsecret support. qtkeychain now has an option called withLibsecret which defaults to false. For nextcloud-client and owncloud-client, qtkeychain is built with withLibsecret = true.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @caugner @Ma27 @qknight