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

refactor(build): Separate thrift and arrow install on Linux #12034

Closed
wants to merge 1 commit into from

Conversation

czentgr
Copy link
Collaborator

@czentgr czentgr commented Jan 7, 2025

This PR addresses two issues.

  1. Address downloading reliability from archive.apache.org by switching to github.
  2. Remove the bundling of thrift from arrow and make it a separate install.

This follows a PR where the bundled version of arrow still bundles thrift but has changed download locations.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 7, 2025
Copy link

netlify bot commented Jan 7, 2025

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 5dc1212
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/679017f0eb7aac0008d4c2eb

@czentgr czentgr force-pushed the cz_setup_thrift branch 3 times, most recently from c2feb06 to 09c88c3 Compare January 9, 2025 21:57
@czentgr czentgr marked this pull request as ready for review January 9, 2025 23:54
@czentgr czentgr requested a review from assignUser as a code owner January 9, 2025 23:54
@majetideepak
Copy link
Collaborator

@czentgr verified the build works as expected with this change.

@majetideepak majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jan 10, 2025
Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

LGTM
Should we also do it in the bundled build? I guess not really necessary there.

@facebook-github-bot
Copy link
Contributor

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@kagamiori
Copy link
Contributor

Hi @czentgr, could you please rebase the PR onto the latest main? It's needed for merging. Thanks!

This PR addresses two issues.
1. Address downloading reliability from archive.apache.org by switching to github.
2. Remove the bundling of thrift from arrow and make it a separate install.

This follows a PR where the bundled version of arrow still bundles thrift but has
changed download locations.
@czentgr
Copy link
Collaborator Author

czentgr commented Jan 21, 2025

@kagamiori FYI, I've rebased again. Thanks!

@facebook-github-bot
Copy link
Contributor

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kagamiori merged this pull request in 8480a60.

Copy link

Conbench analyzed the 0 benchmark runs that triggered this notification.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants