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

pinentry: remove multiple outputs #133542

Merged
merged 2 commits into from Mar 9, 2024
Merged

pinentry: remove multiple outputs #133542

merged 2 commits into from Mar 9, 2024

Conversation

fpletz
Copy link
Member

@fpletz fpletz commented Aug 11, 2021 •
edited

Motivation for this change

Previously pinentry was a package with multiple outputs. This caused lots of rebuilds and always needs the dependencies of all flavors to build. Fixes #133156 #124753.

This PR refactors the different pinentry flavors into separate package builds and keeps the old interface for compatibility. As the attribute name pinentry would be misleading now it was renamed to pinentryFlavors. The old multiple outputs based interface is aliased for compatibility since this was not only used in the gnupg NixOS module but also the corresponding home-manager module.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 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.

@fpletz fpletz requested review from roberth and ttuegel August 11, 2021 20:42
@fpletz fpletz mentioned this pull request Aug 11, 2021
Closed
9 tasks
@ofborg ofborg bot requested review from mmahut, np and peterhoeg August 11, 2021 20:54
@fpletz fpletz force-pushed the refactor/pinentry-remove-multiple-outputs branch from e9aa070 to 010aa08 Compare August 11, 2021 21:54
@r-rmcgibbo
Copy link

r-rmcgibbo commented Aug 12, 2021 •
edited

Result of nixpkgs-review pr 133542 at 010aa08a run on x86_64-linux 1

10 packages built successfully:
  • kwalletcli
  • nixos-install-tools
  • passExtensions.pass-tomb
  • pinentry-emacs (pinentry-curses)
  • pinentry-gnome
  • pinentry-gtk2
  • pinentry-qt
  • python38Packages.trezor_agent
  • trezor_agent (python39Packages.trezor_agent)
  • tomb
6 suggestions:
  • warning: missing-patch-comment

    Consider adding a comment explaining the purpose of this patch on the line preceeding.
    Near pkgs/tools/security/pinentry/default.nix:48:9:

       |
48 |         ./autoconf-ar.patch
   |         ^
  • warning: build-tools-in-build-inputs

    sudo is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/os-specific/linux/tomb/default.nix:16:3:

       |
16 |   buildInputs = [ sudo zsh pinentry-curses ];
   |   ^
  • warning: unclear-gpl

    gpl3 is a deprecated license, please check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

    Near pkgs/development/python-modules/trezor_agent/default.nix:33:5:

       |
33 |     license = licenses.gpl3;
   |     ^
  • warning: unclear-gpl

    gpl3 is a deprecated license, please check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

    Near pkgs/os-specific/linux/tomb/default.nix:40:5:

       |
40 |     license     = licenses.gpl3;
   |     ^
  • warning: missing-patch-comment

    Consider adding a comment explaining the purpose of this patch on the line preceeding.
    Near pkgs/tools/security/pinentry/default.nix:50:9:

       |
50 |         (fetchpatch {
   |         ^
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/os-specific/linux/tomb/default.nix:29:3:

       |
29 |   installPhase = ''
   |   ^

Result of nixpkgs-review pr 133542 at 010aa08a run on aarch64-linux 1

9 packages built successfully:
  • nixos-install-tools
  • passExtensions.pass-tomb
  • pinentry-emacs (pinentry-curses)
  • pinentry-gnome
  • pinentry-gtk2
  • pinentry-qt
  • python38Packages.trezor_agent
  • trezor_agent (python39Packages.trezor_agent)
  • tomb
1 suggestion:
  • warning: unclear-gpl

    gpl3 is a deprecated license, please check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

    Near pkgs/development/python-modules/trezor_agent/default.nix:33:5:

       |
33 |     license = licenses.gpl3;
   |     ^

@lheckemann
Copy link
Member

Looks like a sensible approach to me. I think we can leave out the compatibility hack if we also have a corresponding home-manager PR.

@fpletz fpletz force-pushed the refactor/pinentry-remove-multiple-outputs branch from 010aa08 to a6fe712 Compare August 14, 2021 14:38
@fpletz
Copy link
Member Author

fpletz commented Aug 14, 2021

Thanks for the feedback. I've updated the PR and resolved the conflict with master (the qt variant does not build on aarch64-darwin).

I will open a home-manager PR later and remove the alias.

@fpletz fpletz force-pushed the refactor/pinentry-remove-multiple-outputs branch from a6fe712 to dbfee4b Compare August 14, 2021 14:43
Ma27
Ma27 reviewed Aug 16, 2021
View reviewed changes
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.

Apart from the two comments below this seems reasonable to me.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/aliases.nix Outdated Show resolved Hide resolved
@Ma27
Copy link
Member

Ma27 commented Sep 12, 2021

@fpletz may I ask what's the status here? :)

@stale

This comment was marked as outdated.

Sign in to view
@SuperSandro2000 SuperSandro2000 mentioned this pull request Mar 7, 2024
Merged
13 tasks
@SuperSandro2000
Copy link
Member

@ofborg eval

@ofborg ofborg bot requested a review from arthsmn March 8, 2024 21:25
@SuperSandro2000 SuperSandro2000 force-pushed the refactor/pinentry-remove-multiple-outputs branch from 2520c2e to a270c43 Compare March 8, 2024 22:09
@SuperSandro2000
Copy link
Member

I've just noticed that we also need to add a pinentry to plasma6 and I just did that.

@SuperSandro2000
Copy link
Member

Here goes nothing 🎉

@SuperSandro2000 SuperSandro2000 merged commit c86e8fd into NixOS:master Mar 9, 2024
22 checks passed
@sanderhollaar sanderhollaar mentioned this pull request Mar 10, 2024
Closed
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 10, 2024
@ambroisie
gpg-agent: migrate to 'pinentryPackage'
c8c7d85 
This follows upstream's module change [1], which allows setting any
package as a pinentry program.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 10, 2024
@ambroisie
rbw: simplify 'pinentry' type
7c9b4f4 
Following some upstream changes [1], it's now possible to use a simplified
package type for the option.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 10, 2024
@ambroisie
rbw: simplify 'pinentry' type
5513c26 
Following some upstream changes [1], it's now possible to use a simplified
package type for the option.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 10, 2024
@ambroisie
gpg-agent: migrate to 'pinentryPackage'
cc2fa8e 
This follows upstream's module change [1], which allows setting any
package as a pinentry program.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 10, 2024
@ambroisie
rbw: simplify 'pinentry' type
4000687 
Following some upstream changes [1], it's now possible to use a simplified
package type for the option.

[1]: NixOS/nixpkgs#133542
@flokli
Copy link
Contributor

flokli commented Mar 10, 2024

This broke eval:


       error: undefined variable 'pinentryFlavor'

       at /nix/store/ngy0l9sly1qqg049584bwbiz9bb5x1mv-nixpkgs-src/nixos/modules/services/security/yubikey-agent.nix:40:49:

           39|     # yubikey-agent package
           40|     systemd.user.services.yubikey-agent = mkIf (pinentryFlavor != null) {
             |                                                 ^
           41|       path = [ config.programs.gnupg.agent.pinentryPackage ];
❯

@flokli flokli mentioned this pull request Mar 10, 2024
Merged
13 tasks
@flokli
Copy link
Contributor

flokli commented Mar 10, 2024

Fix in #294771.

ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 11, 2024
@ambroisie
gpg-agent: migrate to 'pinentryPackage'
0790d61 
This follows upstream's module change [1], which allows setting any
package as a pinentry program.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 11, 2024
@ambroisie
rbw: simplify 'pinentry' type
3e90821 
Following some upstream changes [1], it's now possible to use a simplified
package type for the option.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 12, 2024
@ambroisie
rbw: simplify 'pinentry' type
f15e939 
Following some upstream changes [1], it's now possible to use a simplified
package type for the option.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 12, 2024
@ambroisie
gpg-agent: migrate to 'pinentryPackage'
f7960db 
This follows upstream's module change [1], which allows setting any
package as a pinentry program.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 12, 2024
@ambroisie
rbw: simplify 'pinentry' type
0293ff7 
Following some upstream changes [1], it's now possible to use a simplified
package type for the option.

[1]: NixOS/nixpkgs#133542
ambroisie added a commit to ambroisie/home-manager that referenced this pull request Mar 12, 2024
@ambroisie
rbw: simplify 'pinentry' type
13d0b04 
Following some upstream changes [1], it's now possible to use a simplified
package type for the option.

[1]: NixOS/nixpkgs#133542
rycee pushed a commit to ambroisie/home-manager that referenced this pull request Mar 14, 2024
@ambroisie @rycee
gpg-agent: migrate to 'pinentryPackage'
01e4a51 
This follows upstream's module change [1], which allows setting any
package as a pinentry program.

[1]: NixOS/nixpkgs#133542
rycee pushed a commit to ambroisie/home-manager that referenced this pull request Mar 14, 2024
@ambroisie @rycee
rbw: simplify 'pinentry' type
1ab3cec 
Following some upstream changes [1], it's now possible to use a simplified
package type for the option.

[1]: NixOS/nixpkgs#133542
@ners ners mentioned this pull request Mar 17, 2024
Merged
aij added a commit to aij/aij-nixos-config that referenced this pull request Mar 17, 2024
@aij
desktop: Switch between pinentryFlavor and pinentryPackage for compat…
9c94050 
…ibility

NixOS/nixpkgs#133542 removed pinentryFlavor
but it's still needed on NixOS 23.11 since the new option wasn't
added in advance of removing the old one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@Ma27 Ma27 Ma27 left review comments

@roberth roberth roberth approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@ambroisie ambroisie ambroisie approved these changes

@arthsmn arthsmn arthsmn approved these changes

@ttuegel ttuegel Awaiting requested review from ttuegel

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@SCOTT-HAMILTON SCOTT-HAMILTON Awaiting requested review from SCOTT-HAMILTON

@mmahut mmahut Awaiting requested review from mmahut

@np np Awaiting requested review from np

@peterhoeg peterhoeg Awaiting requested review from peterhoeg

@veprbl veprbl Awaiting requested review from veprbl

Assignees
No one assigned
Labels
6.topic: LXQt The Lightweight Qt Desktop Environment 6.topic: nixos 6.topic: python 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: 11-100 12.approvals: 3+ 12.approved-by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pinentry: excessive derivation closure size