fix(build): Add explicit dependency for fastfloat instead of implicit from folly#15322
fix(build): Add explicit dependency for fastfloat instead of implicit from folly#15322MBkkt wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
MBkkt
force-pushed
the
mbkkt/fix-if-strange-folly
branch
from
dbb1df9 to
a234072
Compare
jinchengchenghh
left a comment
jinchengchenghh
left a comment
There was a problem hiding this comment.
Thanks for your fix!
|
@jinchengchenghh Does it work for you in IBM? I'm not sure will or not it works with IBM vcpkg setup. I think changes are anyway better to be merged, because be explicit about dependency is better (also FastFloat system and Folly from source didn't work before these changes). But I want to understand do we need something else to make it works for IBM or not |
|
@assignUser can you review this please? |
| velox_set_source(FastFloat) | ||
| velox_resolve_dependency(FastFloat CONFIG REQUIRED) |
There was a problem hiding this comment.
without this it's impossible to use system fast float.
Because only FindFastFloat.cmake provided
There was a problem hiding this comment.
The https://github.com/facebookincubator/velox/blob/main/CMake/resolve_dependency_modules/fastfloat.cmake#L26 name it to FASTFLOAT, which name should we use? I'm not familiar with the Velox build.
There was a problem hiding this comment.
The fastfloat.cmake will replace FindFastFloat.cmake
There was a problem hiding this comment.
I think there're two cases, both checked in Velox CI:
- installed system fast-float, in such case FindFastFloat.cmake will be used
- from source code build fast-float, in such case fastfloat.cmake will be used, because
velox_build_dependencyis case insensitive.
https://github.com/facebookincubator/velox/blob/main/CMake/resolve_dependency_modules/fastfloat.cmake#L26 name it to FASTFLOAT,
This is expected, because for velox_resolve_dependency_url argument should be written in UPPER_CASE
There was a problem hiding this comment.
^ Yes exactly, sorry for the late answer. Thanks for the change, I didn't realize we where using fastfloat transitively!
|
It could work for IBM. |
|
I have verified in Gluten, this PR can fix the build issue https://github.com/apache/incubator-gluten/actions/runs/18937504151/job/54067911462?pr=10978 This blocks Gluten daily rebase, could you help prioritize this PR? Thanks! @majetideepak @kgpai @czentgr |
majetideepak
left a comment
majetideepak
left a comment
There was a problem hiding this comment.
@MBkkt, @jinchengchenghh I see we use fast_float in Velox here
./velox/functions/prestosql/DateTimeFunctions.h:#include <fast_float/fast_float.h>
Should we add a dependency to the usages?
|
@zhouyuan imported this issue as ibm/velox #15322 |
majetideepak
changed the title
majetideepak
added
the
ready-to-merge
|
@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this in D85881806. |
|
@kevinwilfong merged this pull request in 66446ac. |
6 participants
Otherwise if folly target for some reason doesn't provide fastfloat we will fail to build