Skip to content

Revert "pinentry: drop gtk2"#276143

Merged
fpletz merged 1 commit intoNixOS:masterfrom
lheckemann:revert-pinentry-gtk2-removal
Jan 15, 2024
Merged

Revert "pinentry: drop gtk2"#276143
fpletz merged 1 commit intoNixOS:masterfrom
lheckemann:revert-pinentry-gtk2-removal

Conversation

@lheckemann
Copy link
Member

This reverts commit edb8b79.

Description of changes

Revert the removal of the gtk2 flavour in #270266, since people are still using the gtk2 pinentry for reasons.

gtk2 still won't end up in the runtime closure of systems not using it, but something like #133542 might be a more principled approach that removes it from the build-time closure too.

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.

@ofborg ofborg bot requested review from fpletz and ttuegel December 22, 2023 22:44
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Dec 22, 2023
@SuperSandro2000
Copy link
Member

since people are still using the gtk2 pinentry for reasons.

Why would you want to still use it? It is since 3 years deprecated and there is no DE using it anymore.

@lheckemann
Copy link
Member Author

It's the default in home-manager:

  services.gpg-agent.pinentryFlavor
      Which pinentry interface to use. If not null, it sets pinentry-program in gpg-agent.conf. Beware that pinentry-gnome3 may not work on non-Gnome systems. You can fix it by
      adding the following to your system configuration:

          services.dbus.packages = [ pkgs.gcr ];

      For this reason, the default is gtk2 for now.

      Type: null or one of “curses”, “tty”, “gtk2”, “emacs”, “gnome3”, “qt”

      Default: "gtk2"

      Example: "gnome3"

      Declared by:
          <home-manager/modules/services/gpg-agent.nix>

That's probably worth changing anyway, but in the meantime let's bring back the gtk2 pinentry.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Dec 24, 2023

It's the default in home-manager:

I am not very amused that we are held back by an out of tree project that is setting bad defaults and not properly forwarding the overrides.

I've opened nix-community/home-manager#4805. Lets see how far we get with that instead of undoing this.

@dschrempf
Copy link
Contributor

dschrempf commented Dec 25, 2023

I think this also affects programs.gnupg.agent.pinentryFlavor, which mentions gtk2 as a valid option. (Actually, also the code itself handles gtk2 as a valid option, only the type check throws this error:

      type = types.nullOr (types.enum pkgs.pinentry.flavors);

(I may look at outdated code, I have nixos-unstable checked out here, but then, the pinentry change is alsready on unstable).

EDIT: Actually, there seem to be more problems with programs.gnupg.agent.pinentryFlavor because it mentions ncurses and curses, and I think only the last one is a valid option.

@sg-qwt
Copy link
Contributor

sg-qwt commented Dec 26, 2023

For wayland folks, gave wayprompt a try. It's pretty good, but it's only in nixpkgs-wayland. Would appreciate if anyone can upstream the package in nixpkgs.

@SuperSandro2000
Copy link
Member

@dschrempf I've opened #277221 to fix that. I hope I didn't mix things up.

The linked home-manager PR got merged, so we can close this, right?

@arcnmx arcnmx closed this Dec 28, 2023
@arcnmx arcnmx reopened this Dec 28, 2023
Copy link
Member

@arcnmx arcnmx left a comment

Choose a reason for hiding this comment

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

It's worth noting that the prior PR also silently broke the top-level attribute pkgs.pinentry-gtk2. Unless gtk2 itself is removed from nixpkgs I don't see why this shouldn't continue to be available, and if it is a bottleneck for builds then a separate derivation could be provided as a middle-ground solution instead.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 28, 2023
@SuperSandro2000
Copy link
Member

Unless gtk2 itself is removed from nixpkgs I don't see why this shouldn't continue to be available

It is just no longer available as a default, you can still explicitly opt into this.
Also removing gtk2 in one go is near impossible and a pretty huge task, so we need to start somewhere and default options and dead software are a pretty good start.

if it is a bottleneck for builds then a separate derivation could be provided as a middle-ground solution instead.

I don't think we need to go that way. gpg pinentry is pretty small and pretty fast built.

@fpletz
Copy link
Member

fpletz commented Jan 15, 2024

It is just no longer available as a default, you can still explicitly opt into this.

No, #270266 broke this.

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.

Merging this since there is no use keeping gtk2 broken while #277221 is no ready.

@fpletz fpletz merged commit fcb7c27 into NixOS:master Jan 15, 2024
@lheckemann lheckemann deleted the revert-pinentry-gtk2-removal branch January 15, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants