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

Upgrade folly to v2024.02.26.00 (from v2023.12.04.00) #8950

Closed
wants to merge 1 commit into from

Conversation

pedroerp
Copy link
Contributor

@pedroerp pedroerp commented Mar 4, 2024

No description provided.

@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 Mar 4, 2024
Copy link

netlify bot commented Mar 4, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 70fcc41
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65e66a4c7555b30008320b8a

@facebook-github-bot
Copy link
Contributor

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

@@ -14,9 +14,9 @@
project(Folly)
cmake_minimum_required(VERSION 3.14)

set(VELOX_FOLLY_BUILD_VERSION v2023.12.04.00)
set(VELOX_FOLLY_BUILD_VERSION v2024.02.26.00)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will only set the folly version when you bundle it - if we need to enforce changes everywhere we should change the scripts in scripts/setup-*.sh too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Just changed in the scripts I could find as well.

We should find a way to have this defined in a single place though :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Folly doesn't provide a version file right? So we can't add it to resolve_dependency like for the other deps to enforce the version?

@pedroerp pedroerp force-pushed the velox-update-folly branch from f94b5e1 to 70fcc41 Compare March 5, 2024 00:41
@pedroerp pedroerp changed the title Advance folly version Upgrade folly to v2024.02.26.00 (from v2023.12.04.00) Mar 5, 2024
@facebook-github-bot
Copy link
Contributor

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

@@ -79,7 +79,7 @@ function wget_and_untar {
wget_and_untar https://github.com/gflags/gflags/archive/refs/tags/v2.2.2.tar.gz gflags
wget_and_untar https://ftp.openssl.org/source/openssl-1.1.1k.tar.gz openssl &
wget_and_untar https://boostorg.jfrog.io/artifactory/main/release/1.69.0/source/boost_1_69_0.tar.gz boost &
wget_and_untar https://github.com/facebook/folly/archive/v2023.12.04.00.tar.gz folly &
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets remove this file, as I dont think its being used any more.

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 can do it, but let me do it as a follow up PR to make them more atomic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pedroerp added a commit to pedroerp/velox-1 that referenced this pull request Mar 5, 2024
facebook-github-bot pushed a commit that referenced this pull request Mar 5, 2024
Summary:
According to discussion on:
#8950 (comment)

Pull Request resolved: #8961

Reviewed By: mbasmanova

Differential Revision: D54552525

Pulled By: pedroerp

fbshipit-source-id: 8e07a7e15d6794394e2cfa7cab67e5d0a82a2fb8
@assignUser
Copy link
Collaborator

assignUser commented Mar 21, 2024

Probably best to rebase on main for #8931 to make sure the version is fresh enough to fix it and to trigger the new builds :)

@majetideepak
Copy link
Collaborator

#8931 is reverted here #9197

@facebook-github-bot
Copy link
Contributor

@pedroerp merged this pull request in 9b4a210.

Copy link

Conbench analyzed the 1 benchmark run on commit 9b4a210c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@pedroerp
Copy link
Contributor Author

pedroerp commented Mar 21, 2024

@majetideepak @spershin @amitkdutta @mbasmanova @kgpai @kevinwilfong just merged this as we need a newer version of folly. You may need to re-generate the Prestissimo docker containers next time we advance Presto's Velox submodule, as they are not created automatically yet.

facebook-github-bot pushed a commit that referenced this pull request Mar 26, 2024
Summary:
In the new fbthrift version from #8950  a command fails if `BUILD_SHARED_LIBS` is not explicitly set (because its empty by default).  This PR explicitly sets BUILD_SHARED_LIBS when building fbthrift and also uses the correct flag to disable tests in addition to the default cmake one.

Pull Request resolved: #9204

Reviewed By: kevinwilfong

Differential Revision: D55337270

Pulled By: pedroerp

fbshipit-source-id: 6d77d10e5a1ed0379f52177edf8753246b1e8a6e
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
Summary:
According to discussion on:
facebookincubator#8950 (comment)

Pull Request resolved: facebookincubator#8961

Reviewed By: mbasmanova

Differential Revision: D54552525

Pulled By: pedroerp

fbshipit-source-id: 8e07a7e15d6794394e2cfa7cab67e5d0a82a2fb8
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
…tor#8950)

Summary: Pull Request resolved: facebookincubator#8950

Reviewed By: kgpai

Differential Revision: D54502546

Pulled By: pedroerp

fbshipit-source-id: 59af4f63b32052e967388782bcf88e9f727d8740
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
Summary:
In the new fbthrift version from facebookincubator#8950  a command fails if `BUILD_SHARED_LIBS` is not explicitly set (because its empty by default).  This PR explicitly sets BUILD_SHARED_LIBS when building fbthrift and also uses the correct flag to disable tests in addition to the default cmake one.

Pull Request resolved: facebookincubator#9204

Reviewed By: kevinwilfong

Differential Revision: D55337270

Pulled By: pedroerp

fbshipit-source-id: 6d77d10e5a1ed0379f52177edf8753246b1e8a6e
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants