acme-client: 0.1.16 -> 0.2.4 #71853
Merged
acme-client: 0.1.16 -> 0.2.4 #71853
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The upstream acme-client that used to be at [1] has now been integrated into OpenBSD, and the portable version that it links to at [2] is marked as unmaintained. However, letsencrypt.org links to [3] for a portable version, and indeed, that repository contains a version that has recent activity, so I switched over to that. It is hard to tell what the difference is between the OpenBSD version and what is on Github, and even if that would be easy, there are a lot of Linux-specific changes. This program is dealing with certificates, so I feel it is important to at least check that thare are no obviously unintended differences between the previous version and the new, but I don't know of a good way of doing that at this point. I will continue to investigate before I open a pull request. [1]: https://kristaps.bsd.lv/acme-client/ [2]: https://github.com/kristapsdz/acme-client-portable [3]: https://github.com/graywolf/acme-client-portable
The new source does not include a configure script in the repository, but we can generate it with automake. Also, the new acme-client-portable has an OpenSSL compatibility layer, but that actually breaks building against LibreSSL. Avoid this issue by patching the compatibility layer to be less eager to define things when linking against LibreSSL. I will also submit a pull request for that upstream. I don't expect this to work on Darwin, and the current package suggests it does, but if the upstream (portable) version is no longer maintained, for Darwin, perhaps we should just drop support for it. But maybe it will just work, CI or somebody with a Darwin system will have to try.
My LibreSSL compatibility patch has been merged upstream into acme-client-portable, and version 0.2.4 that includes it has been released, so we can remove the patch here.
|
My compatibility patch has been merged upstream, and version 0.2.4 has been released that includes it, so I updated this PR to drop the patch and use 0.2.4 instead. |
|
Here is a diffstat of the changes from 0.1.16 to 0.2.4: DiffstatThe diff itself is rather large, so I only skimmed through it superficially. A few notable things:
|
|
Is there anything I can do to help move this forward? |
* Replace the manual autoreconf invocation with autoreconfHook. * Remove DEFAULT_CA_FILE, which no longer affects the build.
|
Thanks for the review @c0bw3b, that was very educational. I addressed your comments, please take another look. |
dtzWill
pushed a commit
to dtzWill/nixpkgs
that referenced
this pull request
Nov 6, 2019
* acme-client: 0.1.16 -> 0.2.3 (NixOS#71853) The upstream acme-client that used to be at [1] has now been integrated into OpenBSD, and the portable version that it links to at [2] is marked as unmaintained. However, letsencrypt.org links to [3] for a portable version, and indeed, that repository contains a version that has recent activity, so I switched over to that. It is hard to tell what the difference is between the OpenBSD version and what is on Github, and even if that would be easy, there are a lot of Linux-specific changes. This program is dealing with certificates, so I feel it is important to at least check that thare are no obviously unintended differences between the previous version and the new, but I don't know of a good way of doing that at this point. I will continue to investigate before I open a pull request. [1]: https://kristaps.bsd.lv/acme-client/ [2]: https://github.com/kristapsdz/acme-client-portable [3]: https://github.com/graywolf/acme-client-portable * acme-client: fix Linux build of new upstream The new source does not include a configure script in the repository, but we can generate it with automake. Also, the new acme-client-portable has an OpenSSL compatibility layer, but that actually breaks building against LibreSSL. Avoid this issue by patching the compatibility layer to be less eager to define things when linking against LibreSSL. I will also submit a pull request for that upstream. I don't expect this to work on Darwin, and the current package suggests it does, but if the upstream (portable) version is no longer maintained, for Darwin, perhaps we should just drop support for it. But maybe it will just work, CI or somebody with a Darwin system will have to try. * acme-client: 0.2.3 -> 0.2.4 My LibreSSL compatibility patch has been merged upstream into acme-client-portable, and version 0.2.4 that includes it has been released, so we can remove the patch here. * acme-client: address review feedback * Replace the manual autoreconf invocation with autoreconfHook. * Remove DEFAULT_CA_FILE, which no longer affects the build. (cherry picked from commit a7b49ef)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
A lot has happened since 0.1.16 was packaged. The old upstream website now says:
The linked repository is archived though, and its readme says:
Fortunately, letsencrypt.org links to https://github.com/graywolf/acme-client-portable, which does appear to be a maintained portable version of acme-client, so I took that repository as the new upstream.
It looks like that new acme-client-portable has been modified to build against OpenSSL, but that actually broke LibreSSL compatibility. I included a small patch to restore compatibility. I also submitted that patch upstream.
The new package produces a binary that runs, but I have not tested it extensively. Also, due to the changed upstream, and the portable version being a modified version of the OpenBSD version, it is hard to tell what has changed because of the version bump. I’ll try and see if I can diff the code from the tarballs.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @pmahoney