Skip to content

Conversation

@afh
Copy link
Member

@afh afh commented Feb 7, 2023

Description of changes

In Brief:

  • Add package to install GnuPG 2.2.41 and libgcrypt 1.8.10 both listed as the official Long Term Support (LTS) versions on the GnuPG project's download page
  • Add gnupg-lts and libgcrypt-tls package names
  • Make gnupg-lts depend on libgcrypt-tls
  • Refactor shared code between 22.nix and 23.nix into common.nix

ℹ️ Updating libgcrypt from 1.5.x to 1.8.x seemed okay to me as I did not quickly find any packages depending on it.

In Detail:

The latest change of the GnuPG package (see PR #207071) has been met with both acclaim and some uncertainty.
Folks were uncertain whether NixOS should only provide the latest version or additionally offer an LTS version. The GnuPG 2.2.27 package was was replaced by the GnuPG 2.3.3 package (see 1ee8f77).

A quick investigation shows that several distributions chose the GnuPG LTS version as the default (see my comment on #207071).

@mweinelt's comment mentioning "some interoperability concerns voiced on the distributions@ list, where gnugp with 2.4.0 is incompatible with openpgp implementations following the proposed(?) IETF standard [see draft-ietf-openpgp-crypto-refresh]" inspired me to try and (re-)introduce an LTS version of GnuPG and libgcrypt, the result of which is this PR.

👋 Call for help:

Folks familiar with both or either of the packages, please chime in with additional context that may have been lost and whether the patches that are commented out can be removed or need to be updated.

Until the above call for help has been resolved I believe this PR should continue to be a draft.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@afh afh marked this pull request as draft February 7, 2023 11:50
@afh afh requested review from lsix, peti and vcunat February 7, 2023 11:53
@afh afh mentioned this pull request Feb 7, 2023
13 tasks
@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package labels Feb 7, 2023
@ofborg ofborg bot requested review from fpletz and vrthra February 7, 2023 12:04
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 7, 2023
Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

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

I am for it. I would use the latest version by default, but offer the LTS version for those who want it, just like this PR does. 👍

@afh
Copy link
Member Author

afh commented Feb 7, 2023

Thanks for your comment, @peti. Delighted to hear you are for it 🙂

Would you happen to have any additional context you could share on the state of the patches as mentioned in the 👋 Call for help above by any chance?

@peti
Copy link
Member

peti commented Feb 8, 2023 via email

@doronbehar
Copy link
Contributor

The common.nix file was not pushed.

@afh afh force-pushed the gnupg-libgcrypt-lts branch 2 times, most recently from 17f1e44 to e3f62fb Compare February 13, 2023 10:09
@afh
Copy link
Member Author

afh commented Feb 13, 2023

Thanks for bringing this to my attention, @doronbehar. It seems I'm suffering from a severe case of Monday-morning 🧠 today 😅.

@vcunat vcunat removed their request for review February 13, 2023 12:29
@afh afh force-pushed the gnupg-libgcrypt-lts branch from e3f62fb to bd7af21 Compare February 16, 2023 21:24
@afh
Copy link
Member Author

afh commented Feb 16, 2023

Friendly ping to @lsix, @vrthra, @fpletz about whether you may have more context about the commented tests and whether they can be removed or need to be updated.

@afh afh force-pushed the gnupg-libgcrypt-lts branch 3 times, most recently from 8db05ef to b573b04 Compare February 23, 2023 10:33
@afh
Copy link
Member Author

afh commented Feb 24, 2023

@trofi and @winterqt you've been so very helpful in moving forward with other PRs (#194099, #215699), mind lending a hand here to get the right people involved to get 👀 on this PR?

The main question to resolve is what should happen with the patches that do not apply (cleanly)? Once that is resolved I feel the PR is ready for review.

@trofi
Copy link
Contributor

trofi commented Feb 24, 2023

@trofi and @winterqt you've been so very helpful in moving forward with other PRs (#194099, #215699), mind lending a hand here to get the right people involved to get eyes on this PR?

The main question to resolve is what should happen with the patches that do not apply (cleanly)? Once that is resolved I feel the PR is ready for review.

I am not familiar with gnupg, thus I can only provide general review guidance that might not be in line with maintaners' preferences:

  1. Avoid using common.nix and just inline the contents into all versions you provide.
  2. Check if patches that failed to apply are already upstreamed. Some of them look like backports. But you are introducing older versions. Who nows what we need to backport compared to latest one.
  3. Avoid the suffixes like libgcrypt-lts. Using versioned ones like libgcrypt_1_8 where needed should be enough.

Generally if there are no known problems with latest version and there are no packages that rely on outdated versions I would suggest not to provide outdated versions. Especially for security packages. Upstream should know better when to release.

@afh afh force-pushed the gnupg-libgcrypt-lts branch from b573b04 to c7eb9ad Compare March 8, 2023 18:03
@afh
Copy link
Member Author

afh commented Mar 8, 2023

Thanks for your helpful comments, @trofi, and apologies for taking so long to reply.
I've incorporated your comments into this PR and will look into whether the patches that do not apply have already been merged upstream…

@trofi
Copy link
Contributor

trofi commented Mar 8, 2023

error: undefined variable 'pcsclite'

Can you have a look at ofborg-eval presubmit step on why it fails?

@afh afh force-pushed the gnupg-libgcrypt-lts branch from c7eb9ad to facbd82 Compare March 9, 2023 04:27
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. and removed 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 9, 2023
@afh afh force-pushed the gnupg-libgcrypt-lts branch from 6ab6bae to 858391d Compare March 22, 2023 10:36
@afh
Copy link
Member Author

afh commented Mar 22, 2023

Finally got around to inspect the patches, @trofi.

Obsoleted patches have been removed, other patches have been updated to apply cleanly.

I'm happy to remove the 0001-dirmngr-Only-use-SKS-pool-CA-for-SKS-pool.patch altogether, if folks prefer to not have it lie around unused.
The thing with this patch is that the code it patches is commented out upstream with the note that it has been "[d]isabled […] to due problems with the standard hkps pool."

@doronbehar
Copy link
Contributor

I'm happy to remove the 0001-dirmngr-Only-use-SKS-pool-CA-for-SKS-pool.patch altogether

That sounds great! Could this removal be applied for the non LTS Gnupg?

@afh
Copy link
Member Author

afh commented Mar 22, 2023

Yes, it can, @doronbehar, and I pushed an appropriate change for the removal.

For reference here are links to the upstream source showing the comment and commented code:

@afh afh marked this pull request as ready for review March 29, 2023 12:49
@afh
Copy link
Member Author

afh commented Mar 29, 2023

Thought I had marked this PR as ready for review already. Apologies for the delay.
Friendly ping to @peti, @doronbehar, @trofi in case there isn't a notification about this PR draft now being a proper PR 🙂

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Maybe we should rename the files in both gnupg/ and in libgcrypt/ as default.nix and lts.nix?

Besides that, looks good to me and I approve and encourage the changes, especially the cleanup commits (libgcrypt update & patch cleanup).

@afh
Copy link
Member Author

afh commented Apr 2, 2023

Thanks for the review, @doronbehar. Glad to hear you approve and encourage the suggested changes. 🙂

@trofi suggested to "[a]void the suffixes like libgcrypt-lts. Using versioned ones like libgcrypt_1_8 where needed should be enough." Trofi's comment was about directed at package names; not sure how much of a difference that makes regarding the package filenames.

@doronbehar
Copy link
Contributor

I think this is a matter of taste and the maintainers (who unfortunately don't share their voice here) should decide on those details.

@peti
Copy link
Member

peti commented Apr 3, 2023

I think this is a matter of taste and the maintainers (who unfortunately don't share their voice here) should decide on those details.

I don't feel strongly about the filenames. Using the packages version number in the filename seems to be common practice, but the current scheme works fine, too. No big deal. I'm fine with the PR as it is.

@afh
Copy link
Member Author

afh commented Apr 3, 2023

Thanks for chiming in @peti.

Kindly requesting @fpletz and @vrthra to share their voice on the proposed changes and opinion regarding nix filename and package naming 🙂

@afh
Copy link
Member Author

afh commented Apr 12, 2023

@doronbehar, @trofi, @peti the maintainers have been unresponsive here, do any of you have merge rights to possibly get this in for the 23.05 release?

@peti peti merged commit 3f78d78 into NixOS:master Apr 13, 2023
@trofi
Copy link
Contributor

trofi commented Apr 13, 2023

Oh, isn't 5000+ rebuilds a bit too much for master?

@K900
Copy link
Contributor

K900 commented Apr 13, 2023

32000 rebuilds, no less. Backing out.

, guiSupport ? stdenv.isDarwin, pinentry
}:

with lib;
Copy link
Member

Choose a reason for hiding this comment

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

with lib should not be over the entire file

Copy link
Contributor

Choose a reason for hiding this comment

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

with lib should not be over the entire file

I think it's a left over from the copy pasted nix files, but indeed they all shouldn't use with lib; like this.

@afh
Copy link
Member Author

afh commented Apr 13, 2023

Thanks everyone for chiming in. I'll work on the requested changes and will issue a new PR with staging as its merge base. Stay tuned!

Update: See #226006

@afh afh deleted the gnupg-libgcrypt-lts branch April 13, 2023 10:15
@ajs124 ajs124 mentioned this pull request May 2, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants