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

nixos/gnupg: drop references to gtk2; pinentry-*: build dependent variant with an override instead of choosing the output #277221

Closed
wants to merge 4 commits into from
Closed

nixos/gnupg: drop references to gtk2; pinentry-*: build dependent variant with an override instead of choosing the output #277221

wants to merge 4 commits into from

Conversation

SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Dec 28, 2023 •
edited

Description of changes

This PR basically cleans up all #270266

see #270266 (comment)
Closes #276143
Closes #133156

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@SuperSandro2000 SuperSandro2000 mentioned this pull request Dec 28, 2023
Merged
13 tasks
@con-f-use
Copy link
Contributor

Just wanted to mention again, that all the other pinentry flavors don't handle both tty and graphical prompts as well as gtk2. It's either or with them, so I don't think it should be dropped as a pinentry flavor.

@SuperSandro2000
Copy link
Member Author

all the other pinentry flavors don't handle both tty and graphical prompts as well as gtk2.

I am not sure how gtk2 is realted to tty/curses.

It's either or with them, so I don't think it should be dropped as a pinentry flavor.

It is only dropped as a default entry. You can still configure it to use it.

@SuperSandro2000 SuperSandro2000 mentioned this pull request Dec 29, 2023
Merged
13 tasks
@SuperSandro2000 SuperSandro2000 changed the title nixos/gnupg: drop references to gtk2 nixos/gnupg: drop references to gtk2; pinentry-*: build dependent variant with an override instead of choosing the output Dec 29, 2023
@SuperSandro2000 SuperSandro2000 mentioned this pull request Dec 29, 2023
Closed
@ElvishJerricco
Copy link
Contributor

@SuperSandro2000

I am not sure how gtk2 is realted to tty/curses.

The gtk2 pinentry flavor is the only one that will correctly prompt in the terminal if necessary, or graphically if available. e.g. If I SSH into my machine, I can still use gpg with the gtk2 pinentry flavor, because it will put the prompt in the terminal. Meanwhile, it will still give a graphical prompt if I'm using it from the desktop environment. Other graphical pinentry flavors will always put up the graphical prompt on the desktop, even if you're logged in remotely with SSH.

ambroisie added a commit to ambroisie/nix-config that referenced this pull request Jan 3, 2024
@ambroisie
hosts: nixos: aramis: home: use 'pinentry-qt'
39eba64 
The GTK2 variant has been removed [1].

I may revise this in the future if [2] is merged (I'd like to try
`pinentry-rofi` [3]).

[1]: NixOS/nixpkgs#270266
[2]: NixOS/nixpkgs#277221
[3]: https://github.com/plattfot/pinentry-rofi
jtojnar
jtojnar reviewed Jan 3, 2024
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
pinentry-emacs = pinentry.override { enabledFlavors = [ "emacs" ]; };
pinentry-gtk2 = pinentry.override { enabledFlavors = [ "gtk2" ]; };
pinentry-qt = pinentry.override { enabledFlavors = [ "qt" ]; };
pinentry-gnome3 = pinentry.override { enabledFlavors = [ "gnome3" ]; };
Copy link
Contributor

Choose a reason for hiding this comment

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

I vaguely recall that curses was supposed to be a fallback, if gui could not be started. But now the only reference I found is #71095 (comment), which is not very informative.

@ofborg ofborg bot requested review from fpletz and ttuegel January 4, 2024 02:54
This was referenced Jan 14, 2024
Merged
Merged
@github-actions github-actions bot added 6.topic: qt/kde 6.topic: xfce The Xfce Desktop Environment 6.topic: LXQt The Lightweight Qt Desktop Environment labels Jan 14, 2024
@SuperSandro2000
nixos/gnupg: drop references to gtk2, fix ncurses typo
945f9fe
@SuperSandro2000
pinentry-*: build dependent variant with an override instead of choos…
a880b50 
…ing the output

We don't want to depend on the *enabled* flavors of pinentry but on all
*possible* ones.
@SuperSandro2000
nixos/gnupg: choose the pinentry variant that has the selected flavor…
1de7c1d 
… definitely enabled
@SuperSandro2000
programs/gnupg: move agent.pinentryFlavor to the desktops
daa53d2 
This avoids evaluating all the desktop environments at this place.
@SuperSandro2000
Copy link
Member Author

I cannot test all the changes myself - I think they they are correct but I would appreciate it, if other people could help to verify that.

fpletz
fpletz approved these changes Jan 15, 2024
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Since I'm also using pinentry both graphically and with curses I'm gonna test and merge later today if nobody beats me to it.

fpletz
fpletz requested changes Jan 15, 2024
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Do not merge for now since only setting one enabledFlavor probably does not provide a proper fallback.

@fpletz fpletz mentioned this pull request Jan 15, 2024
Merged
11 tasks
@SuperSandro2000
Copy link
Member Author

Do not merge for now since only setting one enabledFlavor probably does not provide a proper fallback.

I vaguely recall that curses was supposed to be a fallback, if gui could not be started. But now the only reference I found is #71095 (comment), which is not very informative.

Based on this I am not sure that is even working.

@fpletz
Copy link
Member

fpletz commented Jan 15, 2024 •
edited

Based on this I am not sure that is even working.

I'm using this functionality daily and others have also noted they are using this in other PRs regarding the breakage of pinentry-gtk2.

rnhmjoj
rnhmjoj approved these changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@rnhmjoj rnhmjoj 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. I like that DEs set the flavour they need, instead of having gnupg handling it for each and every one.

@fpletz
Copy link
Member

fpletz commented Jan 15, 2024

Some of the useful changes here have been carried over to #133542. Since this PR effectively breaks pinentry's fallback feature, we'll continue in the other PR.

@fpletz fpletz closed this Jan 15, 2024
@SuperSandro2000 SuperSandro2000 deleted the pinentry-gtk2-followup branch January 15, 2024 16:43
@SuperSandro2000
Copy link
Member Author

I'm using this functionality daily and others have also noted they are using this in other PRs regarding the breakage of pinentry-gtk2.

yeah, gtk2 but we don't really know that for the other flavors and I kinda remember that jan some time said, that this might not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth left review comments

@con-f-use con-f-use con-f-use left review comments

@fpletz fpletz fpletz requested changes

@rnhmjoj rnhmjoj rnhmjoj approved these changes

@ambroisie ambroisie ambroisie approved these changes

@lheckemann lheckemann Awaiting requested review from lheckemann

@K900 K900 Awaiting requested review from K900

@ttuegel ttuegel Awaiting requested review from ttuegel

@lobre lobre Awaiting requested review from lobre

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
6.topic: LXQt The Lightweight Qt Desktop Environment 6.topic: nixos 6.topic: qt/kde 6.topic: xfce The Xfce Desktop Environment 8.has: clean-up 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pinentry: excessive derivation closure size
10 participants
@SuperSandro2000 @con-f-use @ElvishJerricco @fpletz @roberth @jtojnar @rnhmjoj @lobre @ambroisie @delroth