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

gnutls: build with unbound/dane support #37005

Closed
wants to merge 3 commits into from

Conversation

schanzen
Copy link
Contributor

  • 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?
  • Is your test running fine brew test <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>)?

I know that the changes are suboptimal, especially the manual changes of CFLAGS/LDFLAGS for unbound.
gnutls does not support supplying the unbound prefix so I am not sure how to solve it.
Also, it would probably be better to make this optional?
Pointers would be helpful.

@javian javian changed the title build with unbound/dane support gnutls: build with unbound/dane support Feb 15, 2019
@@ -31,7 +32,8 @@ def install
--disable-heartbeat-support
--with-p11-kit
]

ENV["CFLAGS"] = "-I/usr/local/Cellar/unbound/1.9.0/include"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure these are needed ? pkg-config and homebrew ought to figure this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking at it. Yes you are correct and I knew about this feature it just wasn't working.
Maybe I got it mixed up.
I added an update that removes the flags and makes the unbound dependency optional.

@schanzen
Copy link
Contributor Author

schanzen commented Feb 15, 2019

EDIT: I see "optional" is frowned upon according to the cookbook. So what is preferred, making it recommended or just a regular depenency?

Now I get:
gnutls:

  • Formulae should not have optional or recommended dependencies
    Error: 1 problem in 1 formula detected

when I check it with brew audit. Why?

@javian
Copy link
Contributor

javian commented Feb 15, 2019

@schanzen options are no longer allowed so if you want it in it needs to be made the default.

@schanzen
Copy link
Contributor Author

Removed the optional. Thanks for the guidance.

@javian
Copy link
Contributor

javian commented Feb 16, 2019

@BrewTestBot test this please

@javian javian added the ready to merge PR can be merged once CI is green label Feb 21, 2019
@javian
Copy link
Contributor

javian commented Feb 21, 2019

@BrewTestBot test this please

@javian
Copy link
Contributor

javian commented Feb 21, 2019

Thanks @schanzen for putting this together and welcome to the Homebrew community! Without people like you it wouldn't flourish and continue to grow and live as it does.

@fxcoudert fxcoudert closed this in 2529d31 Feb 23, 2019
@schanzen schanzen deleted the schanzen branch March 2, 2019 20:03
@lock lock bot added the outdated PR was locked due to age label Apr 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants