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

libsForQt5.qtkeychain: add libsecret support #60327

Merged
merged 1 commit into from Apr 29, 2019

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Apr 27, 2019

Motivation for this change

qtkeychain uses pkg-config to detect whether libsecret is available,
otherwise it just builds a stub object file.

We need libsecret support to allow nextcloud-client storing passwords
on Freedesktop platforms.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@jtojnar
Copy link
Contributor Author

jtojnar commented Apr 27, 2019

I am not sure if setting LD_LIBRARY_PATH in nextcloud-client is necessary anymore. The qtkeychain source code looks like dlopen but it links against the library and libsecret is in its closure.

@Ma27 do you think you could try it without the wrapper?

@jtojnar
Copy link
Contributor Author

jtojnar commented Apr 27, 2019

Okay, I tried without it and it did not work. Not sure why they are linking. (Edit: they seem to use types from the library, though I am not sure why would that require linking.)

Keeping the LD_LIBRARY_PATH saves the token to keyring.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Unfortunately I'm not sure either, when I took the maintainership for nextcloud-client, a wrapper was already used for libgnome-keyring back then.

libsecret is in its closure.

I'm probably missing something right now, but doesn't libsecret need to be in nextcloud-clients's rpath to be found?

pkgs/development/libraries/qtkeychain/default.nix Outdated Show resolved Hide resolved
qtkeychain uses pkg-config to detect whether libsecret is available,
otherwise it just builds a stub object file.

We need libsecret support to allow nextcloud-client storing passwords
on Freedesktop platforms.

I also fixed the Darwin dependencies not being used with Qt5,
even though the build did not complain.
@jtojnar
Copy link
Contributor Author

jtojnar commented Apr 27, 2019

Unfortunately I'm not sure either, when I took the maintainership for nextcloud-client, a wrapper was already used for libgnome-keyring back then.

dlopen will try to find the libraries in LD_LIBRARY_PATH first.

I'm probably missing something right now, but doesn't libsecret need to be in nextcloud-clients's rpath to be found?

Yeah, being in rpath should work too but it did not seem to work for me. And really, the library does not use any symbols from libsecret other than those it loads at runtime – there is need to link against it. It was introduced in frankosterfeld/qtkeychain#107

@Ma27
Copy link
Member

Ma27 commented Apr 28, 2019

Ahh, I see! Thanks a lot for tracking this problem down!

@peterhoeg
Copy link
Member

Should we drop qt4 while at it?

@jtojnar
Copy link
Contributor Author

jtojnar commented Apr 29, 2019

Tomahawk still depends on it. We can drop both in a separate PR.

@jtojnar jtojnar merged commit 98922fe into NixOS:master Apr 29, 2019
@jtojnar jtojnar deleted the qtkeychain-libsecret branch April 29, 2019 06:57
@Ma27
Copy link
Member

Ma27 commented Apr 29, 2019

Thanks! According to the reporter, release-19.03 is also affected, so do we want to backport this?

@jtojnar
Copy link
Contributor Author

jtojnar commented May 7, 2019 via email

@caugner
Copy link
Contributor

caugner commented Jun 15, 2019

@Ma27 Do you have any news regarding backporting? I'm afraid I'm still experiencing #60012 in release-19.03. :/

@tiulpin
Copy link

tiulpin commented Jul 11, 2019

@Ma27 I am experiencing #60012 on 19.03 too.

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