Upgrade FBOS dependencies to 2024.09.16.00 #11018
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
czentgr
force-pushed
the
cz_upgrade_fbos
branch
5 times, most recently
from
e8a868a to
ea38319
Compare
| @@ -32,11 +32,12 @@ by Velox. See details on bundling below. | |||
| | re2 | 2021-04-01 | Yes | | |||
| | fmt | 10.1.1 | Yes | | |||
There was a problem hiding this comment.
I thought this folly version also uses fmt11? Or is it backwards compatible?
There was a problem hiding this comment.
It is backwards compatible. I had originally planned to also upgrade fmt to 11 but there is another PR that needs to be merged first that prepares the code for fmt 11.
Upgrading fmt is next.
There was a problem hiding this comment.
It is backward compatible since Meta internally is still on fmt9 but on latest folly.
There was a problem hiding this comment.
@czentgr two comments.
Can we also wait until the setup script improvement PR lands as this would conflict with that?
| set(VELOX_FOLLY_SOURCE_URL | ||
| "https://github.com/facebook/folly/releases/download/${VELOX_FOLLY_BUILD_VERSION}/folly-${VELOX_FOLLY_BUILD_VERSION}.tar.gz" | ||
| ) | ||
|
|
||
| set(fast_float_SOURCE AUTO) |
There was a problem hiding this comment.
Is this required? AUTO is the default?
There was a problem hiding this comment.
We have to set a source. I was wavering between AUTO or BUNDLED. Given this is only called if Folly is bundled and otherwise was already built with a SYSTEM install of fast_float either one would work. We can set it to BUNDLED to match the way Folly would use this to match it.
czentgr
force-pushed
the
cz_upgrade_fbos
branch
from
ea38319 to
d0a4bbe
Compare
czentgr
force-pushed
the
cz_upgrade_fbos
branch
from
d0a4bbe to
62261c0
Compare
|
@czentgr can you please confirm if you were able to build Velox locally with this change? Unfortunately, CI does not use the latest images to test and we often see build failures after merge when dependencies are updated. |
|
@majetideepak I've tested the following: |
majetideepak
added
the
ready-to-merge
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| set(VELOX_FAST_FLOAT_BUILD_SHA256_CHECKSUM | ||
| 4458aae4b0eb55717968edda42987cabf5f7fc737aee8fede87a70035dba9ab0) | ||
| set(VELOX_FAST_FLOAT_SOURCE_URL | ||
| "https://github.com/fastfloat/fast_float/archive/v${VELOX_FAST_FLOAT_VERSION}.tar.gz" |
There was a problem hiding this comment.
I wonder if this URL path is broken. I think we expect https://github.com/fastfloat/fast_float/archive/refs/tags/v${VELOX_FAST_FLOAT_VERSION}.tar.gz
There was a problem hiding this comment.
This URL works for me. I can navigate in the browser and download it (https://github.com/fastfloat/fast_float/archive/v6.1.6.tar.gz) This is used when you bundle folly. Other than that it should not come up.
Summary: #11018 added install_fast_float, but didn't specify the version by default. The setup failed to download the package with the "tar: Error opening archive: Unrecognized archive format" error. Also the fast_float.cmake introduced by this PR had the wrong VELOX_FAST_FLOAT_SOURCE_URL. It is missing "refs/tags/". This PR fixes these two problems. Fixes #11055 Pull Request resolved: #11056 Reviewed By: DanielHunte Differential Revision: D63344271 Pulled By: Yuhta fbshipit-source-id: 6887b0dce4e5bc38f56949275e14d6135dc07c0b
Summary: This reverts commit 6d1fbf0. I validated locally that reverting this commit fixes the velox_functions_remote_client_test failure. Resolves #11071 Pull Request resolved: #11079 Reviewed By: kgpai Differential Revision: D63323399 Pulled By: pedroerp fbshipit-source-id: a239066f542e3b4867add87a5418c81b7fd3020f
folly brings in a new dependency on fast_float. After discussions with @majetideepak I added it as installable for all platforms and made it bundleable to by used by folly if it is bundled.