Navigation Menu

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.

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

dnscrypt-proxy 1.9.4 #8818

Closed
wants to merge 1 commit into from
Closed

dnscrypt-proxy 1.9.4 #8818

wants to merge 1 commit into from

Conversation

justinmayer
Copy link
Contributor

@justinmayer justinmayer commented Jan 13, 2017 •
edited

This updates dnscrypt-proxy to version 1.9.4. It also moves settings
from the plist to a configuration file, as explained below.

Configuring DNSCrypt settings via arguments in the plist meant that
changes were overwritten on each brew upgrade. Since DNSCrypt recently
added support for configuration files, this formula now uses the
configuration file in order to allow for persistent DNSCrypt settings.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@justinmayer
Copy link
Contributor Author

@ilovezfs / @jedisct1: This pull request updates Homebrew's dnscrypt-proxy formula to utilize the configuration file introduced in version 1.8+. Comments welcome.

@ilovezfs
Copy link
Contributor

There's now 1.9.2 if you'd like to refresh this as a version bump.

@justinmayer
Copy link
Contributor Author

justinmayer commented Jan 16, 2017 •
edited

@ilovezfs: That's great. Thanks for the heads-up. Will do that shortly.

@justinmayer justinmayer changed the title dnscrypt-proxy: Move settings from plist to configuration file dnscrypt-proxy 1.9.2 Jan 16, 2017
@justinmayer
Copy link
Contributor Author

Formula for dnscrypt-proxy 1.9.2 has been updated and is ready for review.

MikeMcQuaid
MikeMcQuaid previously requested changes Jan 18, 2017
View reviewed changes
Formula/dnscrypt-proxy.rb
@@ -24,9 +24,18 @@ class DnscryptProxy < Formula
depends_on "ldns" => :recommended

def install
# Configure an initial resolver
inreplace "dnscrypt-proxy.conf", "ResolverName please-change-the-resolver-name-in-the-config-file", "ResolverName dnscrypt.eu-dk"
Copy link
Member

Choose a reason for hiding this comment

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

Can you split this (and those below) onto 2-3 lines for readability? Thanks!

@ilovezfs
Copy link
Contributor

And it's at 1.9.3 now! This release schedule is breakneck.

@justinmayer
Copy link
Contributor Author

justinmayer commented Jan 19, 2017 via email

@justinmayer justinmayer changed the title dnscrypt-proxy 1.9.2 dnscrypt-proxy 1.9.4 Jan 22, 2017
@justinmayer
Copy link
Contributor Author

@MikeMcQuaid: I made the requested changes (one of which was recently rendered irrelevant). I also updated the formula to yesterday's 1.9.4 release. Would you please review and merge if all looks good here?

@ilovezfs
Copy link
Contributor

On Yosemite, dnscrypt-update-resolvers fails with

yosemitevm:~ brew$ /usr/local/opt/dnscrypt-proxy/bin/dnscrypt-update-resolvers  
+ RESOLVERS_UPDATES_BASE_URL=https://download.dnscrypt.org/dnscrypt-proxy
+ RESOLVERS_LIST_BASE_DIR=/usr/local/Cellar/dnscrypt-proxy/1.9.1/share/dnscrypt-proxy
+ RESOLVERS_LIST_PUBLIC_KEY=RWQf6LRCGA9i53mlYecO4IzT51TGPpvWucNSCh1CBM0QTaLn73Y7GFO3
+ curl -v -L --max-redirs 5 -4 -m 30 --connect-timeout 30 -s https://download.dnscrypt.org/dnscrypt-proxy/dnscrypt-resolvers.csv
*   Trying 37.59.238.213...
* Connected to download.dnscrypt.org (37.59.238.213) port 443 (#0)
* SSL peer handshake failed, the server most likely requires a client certificate to connect
* Closing connection 0

@justinmayer
Copy link
Contributor Author

@ilovezfs: Saw that. Could be due to Yosemite's old version of OpenSSL. Not sure how to resolve that one.

@ilovezfs
Copy link
Contributor

We can do any of these:

  1. depends_on :macos => :el_capitan and drop Yosemite support
  2. depends_on curl => "with-openssl" and have dnscrypt-update-resolvers use that if building with the minisign option and on Yosemite
  3. remove the minisign option on Yosemite

If 2, the minisign option should probably be optional not recommended on Yosemite.

My initial inclination is 1.

@MikeMcQuaid thoughts?

@MikeMcQuaid
Copy link
Member

depends_on curl => "with-openssl" and have dnscrypt-update-resolvers use that if building with the minisign option and on Yosemite
remove the minisign option on Yosemite
If 2, the minisign option should probably be optional not recommended on Yosemite.

I'd prefer one of these but don't have a preference as to which.

@ilovezfs
Copy link
Contributor

Since we were talking the other day about possibly auditing out all => "with-foo" options at some point, if not 1, then probably 3 is best.

@justinmayer
Copy link
Contributor Author

justinmayer commented Jan 24, 2017 via email

@ilovezfs
Copy link
Contributor 

depends_on "minisign" => :recommended if MacOS.version > :yosemite

or

depends_on "minisign" => :recommended if MacOS.version >= :el_capitan

@MikeMcQuaid
Copy link
Member

@ilovezfs I defer to you here.

@ilovezfs
Copy link
Contributor

@BrewTestBot test this please

@justinmayer
Copy link
Contributor Author

@ilovezfs: Thanks for the suggested modifications. I just pushed those changes and am awaiting test results.

@ilovezfs
Copy link
Contributor

@justinmayer actually I already merged that change

@justinmayer
dnscrypt-proxy 1.9.4
fd7a986 
This updates dnscrypt-proxy to version 1.9.4. It also moves settings
from the plist to a configuration file, as explained below.

Configuring DNSCrypt settings via arguments in the plist meant that
changes were overwritten on each `brew upgrade`. Since DNSCrypt recently
added support for configuration files, this formula now uses the
configuration file in order to allow for persistent DNSCrypt settings.

Refs Homebrew#8396
@justinmayer
Copy link
Contributor Author

Tests are passing now. Looks like all is ready to merge. 👍

@ilovezfs ilovezfs dismissed MikeMcQuaid’s stale review January 24, 2017 14:49

referenced line is no longer in the PR

@ilovezfs
Copy link
Contributor

Shipped! Thanks for the good work here.

@ilovezfs ilovezfs closed this in dde7062 Jan 24, 2017
@justinmayer
Copy link
Contributor Author

Likewise! Nice team effort. 🎉

@jedisct1
Copy link
Contributor

Good job everyone! 👍

@MikeMcQuaid MikeMcQuaid mentioned this pull request Jan 26, 2017
Merged
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ilovezfs ilovezfs ilovezfs approved these changes

@MikeMcQuaid MikeMcQuaid MikeMcQuaid left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@justinmayer @ilovezfs @MikeMcQuaid @jedisct1