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.

Sign up for GitHub

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

Jump to bottom

kwallet: support GPG-encrypted wallets #23622

Merged
merged 1 commit into from Apr 2, 2017
Merged

kwallet: support GPG-encrypted wallets #23622

merged 1 commit into from Apr 2, 2017

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Mar 7, 2017 •
edited

Motivation for this change

I migrated to NixOS and could not open my KWallet wallets because they were gpg-encrypted and KWallet was not built with gpgme. (For an explanation see e.g. https://forum.kde.org/viewtopic.php?f=289&t=138507.)

KWallet compilation failed with gpgme-1.8.0 because gpgme had broken its cmake definitions in b2c07bd4, between 1.7.1 and 1.8.0, and have fixed them in 572c1aac and 2e661b9e, after 1.8.0.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@orivej, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fpletz, @vcunat and @ttuegel to be potential reviewers.

joachifm
joachifm reviewed Mar 8, 2017
View reviewed changes
pkgs/development/libraries/gpgme/default.nix Outdated
@@ -9,11 +9,13 @@ let
gpgProgram = if useGnupg1 then "gpg" else "gpg2";
in
stdenv.mkDerivation rec {
name = "gpgme-1.8.0";
name = "gpgme-1.7.1";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be better to add a gpgme_1_7 package instead of downgrading the main version; there are other users of gpgme in the tree after all :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or do you mean 1.8 is broken in general?

Copy link
Contributor Author

@orivej orivej Mar 8, 2017 •
edited

Choose a reason for hiding this comment

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

No, not in general, although 1.8.0 was particularly hostile to MacOS :-) — they meant to do this, which is a patch we might as well add rather than splitting gpgme_1_7 out of gpgme, and see nixpkgs workaround for another regression in 1.8.0. Both of these were fixed in gpgme master, and I believe we should better wait for 1.8.1 or 1.9.0 on 1.7.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @Fuuzetsu as the listed gpgme maintainer

Copy link
Member

Choose a reason for hiding this comment

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

Both of these were fixed in gpgme master, and I believe we should better wait for 1.8.1 or 1.9.0 on 1.7.1.

gpgme 1.9.0 is out (#24491) and I guess we can merge it soon (just wasn't sure about useGnupg1 change). I guess we can merge this shortly after the upgrade 😄.

primeos
primeos reviewed Mar 30, 2017
View reviewed changes
pkgs/development/libraries/gpgme/default.nix Outdated
@@ -30,9 +32,6 @@ stdenv.mkDerivation rec {
NIX_CFLAGS_COMPILE =
with stdenv; lib.optional (system == "i686-linux") "-D_FILE_OFFSET_BITS=64";

AM_CXXFLAGS =
with stdenv; lib.optional (isDarwin) "-D_POSIX_C_SOURCE=200809L";

Copy link
Member

@primeos primeos Mar 30, 2017 •
edited

Choose a reason for hiding this comment

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

BTW: Why did you drop this? Should I include that change for 1.9.0? I'm actually a bit confused because this is for macOS/Darwin only and according to you PR you didn't build this on macOS - are you sure that works?

Edit: nvm, found it: https://bugs.gnupg.org/gnupg/issue2910

@orivej
Copy link
Contributor Author

orivej commented Mar 30, 2017

I'll rebase this on top of master once #24491 is merged.

@orivej
Copy link
Contributor Author

orivej commented Apr 1, 2017

Rebased.

@primeos
Copy link
Member

primeos commented Apr 2, 2017 •
edited

Looks good to me 😄.
I can't verify all 151 rebuilds (would take a bit too long... :D) but I assume it's pretty unlikely that it would break anything.

Note (Edited): I actually verified the first 98 packages (they all succeeded). kdevelop is actually broken on master (i.e. it's not related to this commit) - @ambrop72 could you have a look?

@joachifm could you please merge this if it looks ok to you as well?

[x] Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"

That command only works as expected if you didn't already commit your changes (i.e. it works on the changes you can see with git diff). If you've already committed the changes you should e.g. run nix-shell -p nox --run "nox-review wip --against HEAD~" or nix-shell -p nox --run "nox-review pr 23622" 😉. But you don't have to run this in this case as it takes way too long.

@ambrop72
Copy link
Contributor

ambrop72 commented Apr 2, 2017

@primeos The kdevelop issue is due to a problem in qt, I am testing a fix then I will make a PR.

@joachifm joachifm merged commit 5daf6b3 into NixOS:master Apr 2, 2017
@orivej orivej deleted the kwallet branch April 7, 2017 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joachifm joachifm joachifm left review comments

@primeos primeos primeos left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@orivej @mention-bot @primeos @ambrop72 @joachifm