Skip to content

thrift 0.12.0 - #35796

Closed
taraspos wants to merge 3 commits into
Homebrew:masterfrom
taraspos:patch-2
Closed

thrift 0.12.0#35796
taraspos wants to merge 3 commits into
Homebrew:masterfrom
taraspos:patch-2

Conversation

@taraspos

@taraspos taraspos commented Jan 8, 2019

Copy link
Copy Markdown
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>)?

@fxcoudert

fxcoudert commented Jan 8, 2019

Copy link
Copy Markdown
Member
  • Please squash both commits into a single one, thrift 0.12.0
  • Add to this commit a rename of Aliases/[email protected] to Aliases/[email protected]
  • In a second commit, bump revision of osquery

@taraspos

taraspos commented Jan 8, 2019

Copy link
Copy Markdown
Contributor Author

@fxcoudert
thanks for the feedback, will do.
But I didn't get the one about osquery.
Do you mean update https://github.com/Homebrew/homebrew-core/blob/master/Formula/osquery.rb to 3.3.1?

@taraspos taraspos changed the title Update thrift package to 0.12.0 thrift 0.12.0 Jan 8, 2019
@taraspos

taraspos commented Jan 8, 2019

Copy link
Copy Markdown
Contributor Author

About the osquery, it is not published on the official downloads page yet https://osquery.io/downloads/official, and I can't find the sha256for the package anywhere else.

@fxcoudert

fxcoudert commented Jan 8, 2019

Copy link
Copy Markdown
Member

I mean in osquery, bump revision 1 to revision 2, in a separate commit with title osquery: revision for thrift

@taraspos

taraspos commented Jan 8, 2019

Copy link
Copy Markdown
Contributor Author

All done

@fxcoudert

fxcoudert commented Jan 8, 2019

Copy link
Copy Markdown
Member

Removing the seldom-used options, per #31510

==> install (30 days)
Index | Name (with options)                                   | Count |  Percent
-----:|-------------------------------------------------------|------:|--------:
1     | thrift                                                | 1,439 |   97.30%
2     | thrift --HEAD                                         |    21 |    1.42%
3     | thrift --with-java                                    |     7 |    0.47%
4     | thrift --with-python@2                                |     7 |    0.47%

@fxcoudert fxcoudert added ready to merge PR can be merged once CI is green and removed ready to merge PR can be merged once CI is green labels Jan 8, 2019
@fxcoudert

fxcoudert commented Jan 10, 2019

Copy link
Copy Markdown
Member

osquery build failure…

@taraspos

taraspos commented Jan 15, 2019

Copy link
Copy Markdown
Contributor Author

@fxcoudert any ideas what can be done here? 😕

@taraspos
taraspos force-pushed the patch-2 branch 2 times, most recently from a1031e3 to 9430210 Compare January 24, 2019 08:57
@SMillerDev SMillerDev added upstream issue An upstream issue report is needed build failure CI fails while building the software labels Jan 24, 2019
@SMillerDev

SMillerDev commented Jan 24, 2019

Copy link
Copy Markdown
Member

related to #35941

Fokko
Fokko reviewed Jan 24, 2019
View reviewed changes
Comment thread Formula/thrift.rb Outdated

@Fokko Fokko Jan 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are you removing Java, I think it is important to have the Java bindings. If so, you need to remove the env-settings as well.

@SMillerDev SMillerDev Jan 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Barely anyone uses it based on the analytics above and it pulls in a huge dependency, I'd say this is a valid reason to remove it.

@Fokko Fokko mentioned this pull request Jan 24, 2019
Closed
5 tasks done
Fokko
Fokko reviewed Jan 24, 2019
View reviewed changes
Comment thread Formula/thrift.rb Resolved
@Fokko Fokko mentioned this pull request Jan 31, 2019
Merged
@Fokko

Fokko commented Feb 4, 2019

Copy link
Copy Markdown
Contributor

Any update on this?

@adamlc

adamlc commented Feb 25, 2019

Copy link
Copy Markdown

Would love an update if there is one 👍

@linguofeng

linguofeng commented Mar 1, 2019

Copy link
Copy Markdown

Any updates?

@taraspos

taraspos commented Mar 1, 2019

Copy link
Copy Markdown
Contributor Author

Sorry guys, I have no idea what I can do here. The most I can try is to rerun the build, maybe it will work now ¯_(ツ)_/¯.

The upstream issue seems to be ignored.

@fxcoudert

fxcoudert commented Mar 4, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

The osquery build should be fixed with the new version: 8b42b60

Can you:

  • rebase the thrift update onto master?
  • bump the osquery revision, in a separate commit?

That should do the trick

@fxcoudert fxcoudert removed build failure CI fails while building the software upstream issue An upstream issue report is needed labels Mar 4, 2019
@fxcoudert

fxcoudert commented Mar 4, 2019

Copy link
Copy Markdown
Member

Thanks @trane9991 ! 🍸

@fxcoudert fxcoudert closed this in ea00488 Mar 4, 2019
@lock lock Bot added the outdated PR was locked due to age label Apr 3, 2019
@lock lock Bot locked as resolved and limited conversation to collaborators Apr 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@SMillerDev SMillerDev SMillerDev left review comments
+1 more reviewer
@Fokko Fokko Fokko left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

outdated PR was locked due to age

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@taraspos @fxcoudert @SMillerDev @Fokko @adamlc @linguofeng