Fix fast_float installation problems #11056
Conversation
default. The setup-macos.sh failed to download the package. We noticed that the FAST_FLOAT_VERSION variable, unlike in setup-ubuntu.sh and setup-centos.sh, was not set in setup-macos.sh. This commit fixes this issue by adding this variable.
"refs/tags/". This commit fixes this bug.
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
| @@ -17,7 +17,7 @@ set(VELOX_FAST_FLOAT_VERSION 6.1.6) | |||
| 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" | |||
| "https://github.com/fastfloat/fast_float/archive/refs/tags/v${VELOX_FAST_FLOAT_VERSION}.tar.gz" | |||
There was a problem hiding this comment.
@yingsu00 the original url seems valid. Can you check?
I see that Macos CI is passing as well.
There was a problem hiding this comment.
The URL with refs/tags/ would work too but the current one should too. I had removed the path to match the other URLs that are similar.
I had tested the bundling which would download from the URL as well and that worked fine.
The URL is https://github.com/fastfloat/fast_float/archive/v6.1.6.tar.gz
There was a problem hiding this comment.
Probably we should revert the cmake changes as original, while remove refs/tags/ in the setup script so be consistent. I prefer shorter URLs as possible.
Line 146 in 4e45bc5
The setup scripts is not that consistent about the url though among other package download URLs.
There was a problem hiding this comment.
I did check. The url without refs/tags/ returns 404
There was a problem hiding this comment.
It works today. When i tried it on Friday it didn't work
| @@ -43,6 +43,7 @@ MACOS_VELOX_DEPS="bison flex gflags glog googletest icu4c libevent libsodium lz4 | |||
| MACOS_BUILD_DEPS="ninja cmake" | |||
| FB_OS_VERSION="v2024.09.16.00" | |||
| FMT_VERSION="10.1.1" | |||
| FAST_FLOAT_VERSION="v6.1.6" | |||
There was a problem hiding this comment.
Local quick workaround is to install it through brew or use folly bundling - or add this manually. How did I miss this.
There was a problem hiding this comment.
What I don't get was why the CI for MacOS succeeded before. Could it be the FAST_FLOAT_VERSION variable was set in Linux and somehow the value remains?
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. |
This reverts commit 0dd3a5d.
#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