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

Add optional dependency for on p11-kit for gnutls #5427

Closed
wants to merge 1 commit into from

Conversation

toonetown
Copy link
Contributor

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • Does your submission pass brew audit --new-formula <formula> (after doing brew install <formula>)?

After the discussion and comment at #5390 (comment), this PR adds an optional dependency on p11-kit. Similar changes are included within #5416, however, this change is limited to JUST gnutls.

@tschoonj
Copy link
Contributor

I already did this in PR #5416

@toonetown
Copy link
Contributor Author

@tschoonj
I already did this in PR #5416

Yes - I mentioned that...however, that change is a modification of multiple formulas - while this change is just a modification of gnutls. IMO, it is nicer to have them separated out into different commits/PRs (but the homebrew maintainers may disagree with me on that...in which case, they can close this one)

@tschoonj
Copy link
Contributor

If you don't to wait until my PR is merged in, you could also use brew pull to get the changes.

@DomT4
Copy link
Member

DomT4 commented Sep 30, 2016

Blocked on resolution of #5434 and/or 46be01e.

@DomT4 DomT4 added do not merge in progress Stale bot should stay away labels Sep 30, 2016
@toonetown
Copy link
Contributor Author

Closing this one in favor of #5416 (which apparently has this change as well)

@toonetown toonetown closed this Oct 21, 2016
@toonetown toonetown deleted the gnutls_p11kit branch October 21, 2016 17:04
@fdelapena
Copy link

@toonetown it looks like #5416 removed p11-kit dependency before merging. Could you reopen this pull request?

@toonetown
Copy link
Contributor Author

If this is appropriate and accepted, the "Do Not Merge" tag may need to be removed (I don't have permissions to change tags)

@toonetown
Copy link
Contributor Author

This pull request is with an "optional" dependency on p11-kit - however, given the comment at #5390 (comment), maybe we should make it a recommended option?

@nmav
As the maintainer of gnutls, I'd strongly suggest making the p11-kit variant the default. The PKCS#11 functionality is essential in modern gnutls versions, especially for primary platforms like OSX which you would like your users to use smart cards, HSMs and the keychaintoken.

@toonetown
Copy link
Contributor Author

Do I bump the "revision" in this formula for a change like this? I would guess the answer would be "yes" if we were to make p11-kit :recommended and "no" if we leave it as :optional. Is that correct?

@MikeMcQuaid
Copy link
Member

Cross-posting from #8274 (comment):

Re: gnutls itself: given the gnutls maintainer has strongly recommended we make it on-by-default I'm fine with doing that, too, now. If we do so it'd be good to also figure out how to include and enable the p11-kit macOS keychain integration you mentioned, too.

If it's :recommended (or even not an option at all) then, yeh, we probably want a revision bump.

@toonetown
Copy link
Contributor Author

@MikeMcQuaid
If we do so it'd be good to also figure out how to include and enable the p11-kit macOS keychain integration you mentioned, too.

I will investigate this more and possibly submit a separate PR.

@toonetown toonetown force-pushed the gnutls_p11kit branch 2 times, most recently from 298163d to 7405af0 Compare December 29, 2016 20:11
@toonetown
Copy link
Contributor Author

I have updated this PR with the dependency as :recommended. If this is acceptable, could we get the do not merge tag removed, and get it reviewed for possible inclusion?

With this change, then change in #8274 is no longer needed.

toonetown added a commit to toonetown/homebrew-core that referenced this pull request Dec 29, 2016
Once PR Homebrew#5427 is merged in and p11-kit support is added to gnutls, we need to bump the revision of openconnect so that it will build with p11-kit support.
@MikeMcQuaid
Copy link
Member

Thanks for your contribution to Homebrew and patience with us here! Without people like you submitting PRs we couldn't run this project. You rock!

For future reference the preferred commit message format is gnutls: add recommended p11-kit dependency.. Please complete the issue template in future PRs where this format is detailed in the linked Contributing guidelines. Thanks!

@toonetown toonetown deleted the gnutls_p11kit branch April 10, 2017 14:47
@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.
Labels
in progress Stale bot should stay away
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants