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

WIP: wdt 1.27.1612021-145-gbc22626 (new formula) #39345

Closed
wants to merge 1 commit into from

Conversation

hjmallon
Copy link
Contributor

@hjmallon hjmallon commented Apr 26, 2019

  • 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>)?

This is very similar to an old pull request here Homebrew/legacy-homebrew#50381

The patch from the old one is no longer needed but other comments on there are not resolved in this version (see https://discourse.brew.sh/t/need-help-making-wdt-formula/4700)

@lembacon lembacon added the new formula PR adds a new formula to Homebrew/homebrew-core label Apr 26, 2019
Formula/wdt.rb Outdated
depends_on "glog"
depends_on "openssl"

resource "folly" do
Copy link
Member

Choose a reason for hiding this comment

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

We already have a folly formula, please depend on that.

Copy link
Contributor Author

@hjmallon hjmallon Apr 28, 2019

Choose a reason for hiding this comment

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

It requires cpp files from Folly to build, which is why it requires folly formula (for headers) and also resources folly (for headers and cpps during build time). I have no idea what the best way to resolve that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think maybe something could be changed upstream to link against a full version of folly rather than building libfolly4wdt, which seems to be a subset of the whole lib. I made an issue here facebook/wdt#194

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a patch here to cover building against full folly and removing the resource. I am going to try to get something similar upstream.

Formula/wdt.rb Show resolved Hide resolved
@hjmallon hjmallon changed the title wdt 1.27.1612021-145-gbc22626 (new formula) WIP: wdt 1.27.1612021-145-gbc22626 (new formula) Apr 29, 2019
@hjmallon
Copy link
Contributor Author

Changed to WIP until I can get patches together here or upstream to build against and link full folly.

@BrewTestBot
Copy link
Member

  • Formulae should not require patches to build. Patches should be submitted and accepted upstream first.

@hjmallon
Copy link
Contributor Author

Thanks everyone for reviewing this. It seems the upstream developers at Facebook are pretty unresponsive. https://github.com/facebook/wdt

I am going to close this for now as there is no sensible way to merge without patches.

@hjmallon hjmallon closed this May 20, 2019
@lock lock bot added the outdated PR was locked due to age label Feb 17, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants