velox
velox copied to clipboard
facebookincubator
[WIP] Add support for installing and build dependencies, install folly using FetchContent
This PR is currently WIP.
The idea is similar to the PR https://github.com/facebookincubator/velox/pull/2412 but using FetchContent instead of EXTERNALPROJECT_ADD to avoid the transient linking issues.
I am pushing the branch so we can start discussing around the different approaches.
It currently fails due to DeprecationWarnings with the OpenSSL being used:
FAILED: velox/functions/prestosql/CMakeFiles/velox_functions_prestosql_impl.dir/Reverse.cpp.o
ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBOOST_ATOMIC_DYN_LINK -DBOOST_CONTEXT_DYN_LINK -DBOOST_DATE_TIME_DYN_LINK -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_REGEX_DYN_LINK -DBOOST_SYSTEM_DYN_LINK -DBOOST_THREAD_DYN_LINK -DFMT_LOCALE -DGFLAGS_IS_A_DLL=0 -I/velox/. -I/velox/velox/external/xxhash -I/velox/_build/release/_deps/folly-src -I/velox/_build/release/_deps/folly-build -I/velox/third_party/xsimd/include -isystem /velox/velox -isystem /velox/velox/external -isystem /velox/velox/external/duckdb -isystem /velox/velox/external/duckdb/tpch/dbgen/include -isystem /velox/third_party/googletest/googletest/include -isystem /velox/third_party/googletest/googletest -mavx2 -mfma -mavx -mf16c -mlzcnt -mbmi2 -std=c++17 -D USE_VELOX_COMMON_BASE -D HAS_UNCAUGHT_EXCEPTIONS -Werror -Wall -Wextra -Wno-unused -Wno-unused-parameter -Wno-sign-compare -Wno-ignored-qualifiers -Wno-implicit-fallthrough -Wno-empty-body -Wno-class-memaccess -Wno-comment -Wno-int-in-bool-context -Wno-redundant-move -Wno-array-bounds -Wno-maybe-uninitialized -Wno-unused-result -Wno-format-overflow -Wno-strict-aliasing -Wno-type-limits -O3 -DNDEBUG -std=gnu++17 -MD -MT velox/functions/prestosql/CMakeFiles/velox_functions_prestosql_impl.dir/Reverse.cpp.o -MF velox/functions/prestosql/CMakeFiles/velox_functions_prestosql_impl.dir/Reverse.cpp.o.d -o velox/functions/prestosql/CMakeFiles/velox_functions_prestosql_impl.dir/Reverse.cpp.o -c /velox/velox/functions/prestosql/Reverse.cpp
In file included from /velox/_build/release/_deps/folly-src/folly/ssl/OpenSSLHash.h:27,
from /velox/./velox/functions/lib/string/StringImpl.h:30,
from /velox/velox/functions/prestosql/Reverse.cpp:20:
/velox/_build/release/_deps/folly-src/folly/ssl/OpenSSLPtrTypes.h:95:53: error: 'void HMAC_CTX_free(HMAC_CTX*)' is deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations]
....
250 | 1, HMAC_CTX_copy(this->ctx_.get(), that.ctx_.get()));
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /velox/_build/release/_deps/folly-src/folly/portability/OpenSSL.h:39,
from /velox/_build/release/_deps/folly-src/folly/ssl/OpenSSLHash.h:26,
from /velox/./velox/functions/lib/string/StringImpl.h:30,
from /velox/velox/functions/prestosql/Reverse.cpp:20:
/usr/include/openssl/hmac.h:49:34: note: declared here
49 | OSSL_DEPRECATEDIN_3_0 __owur int HMAC_CTX_copy(HMAC_CTX *dctx, HMAC_CTX *sctx);
| ^~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
Deploy Preview for meta-velox canceled.
| Name | Link |
|---|---|
| Latest commit | 2425658c111b9430c27df935fa5fc70d6649912f |
| Latest deploy log | https://app.netlify.com/sites/meta-velox/deploys/6333180005a2060008c82c07 |
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
@kgpai @assignUser This is an alternative approach using FetchContent to download/build third party dependencies. This PR is in a rougher state and it does not currently build due to some version issues with OpenSSL but I wanted to share an alternate approach.
The below is fixed now.
I couldn't get rid of the deprecation warning in case of OpenSSL 3.0 installed. I had to add
-Wno-deprecated-declarationsto the known warnings. Maybe something we can work in the future.FAILED: velox/functions/prestosql/CMakeFiles/velox_functions_prestosql_impl.dir/Reverse.cpp.o ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBOOST_ATOMIC_DYN_LINK -DBOOST_CONTEXT_DYN_LINK -DBOOST_DATE_TIME_DYN_LINK -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_REGEX_DYN_LINK -DBOOST_SYSTEM_DYN_LINK -DBOOST_THREAD_DYN_LINK -DFMT_LOCALE -DGFLAGS_IS_A_DLL=0 -I/velox/. -I/velox/velox/external/xxhash -I/velox/_build/release/_deps/folly-src -I/velox/_build/release/_deps/folly-build -I/velox/third_party/xsimd/include -isystem /velox/velox -isystem /velox/velox/external -isystem /velox/velox/external/duckdb -isystem /velox/velox/external/duckdb/tpch/dbgen/include -isystem /velox/third_party/googletest/googletest/include -isystem /velox/third_party/googletest/googletest -mavx2 -mfma -mavx -mf16c -mlzcnt -mbmi2 -std=c++17 -D USE_VELOX_COMMON_BASE -D HAS_UNCAUGHT_EXCEPTIONS -Werror -Wall -Wextra -Wno-unused -Wno-unused-parameter -Wno-sign-compare -Wno-ignored-qualifiers -Wno-implicit-fallthrough -Wno-empty-body -Wno-class-memaccess -Wno-comment -Wno-int-in-bool-context -Wno-redundant-move -Wno-array-bounds -Wno-maybe-uninitialized -Wno-unused-result -Wno-format-overflow -Wno-strict-aliasing -Wno-type-limits -O3 -DNDEBUG -std=gnu++17 -MD -MT velox/functions/prestosql/CMakeFiles/velox_functions_prestosql_impl.dir/Reverse.cpp.o -MF velox/functions/prestosql/CMakeFiles/velox_functions_prestosql_impl.dir/Reverse.cpp.o.d -o velox/functions/prestosql/CMakeFiles/velox_functions_prestosql_impl.dir/Reverse.cpp.o -c /velox/velox/functions/prestosql/Reverse.cpp In file included from /velox/_build/release/_deps/folly-src/folly/ssl/OpenSSLHash.h:27, from /velox/./velox/functions/lib/string/StringImpl.h:30, from /velox/velox/functions/prestosql/Reverse.cpp:20: /velox/_build/release/_deps/folly-src/folly/ssl/OpenSSLPtrTypes.h:95:53: error: 'void HMAC_CTX_free(HMAC_CTX*)' is deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations] .... 250 | 1, HMAC_CTX_copy(this->ctx_.get(), that.ctx_.get())); | ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /velox/_build/release/_deps/folly-src/folly/portability/OpenSSL.h:39, from /velox/_build/release/_deps/folly-src/folly/ssl/OpenSSLHash.h:26, from /velox/./velox/functions/lib/string/StringImpl.h:30, from /velox/velox/functions/prestosql/Reverse.cpp:20: /usr/include/openssl/hmac.h:49:34: note: declared here 49 | OSSL_DEPRECATEDIN_3_0 __owur int HMAC_CTX_copy(HMAC_CTX *dctx, HMAC_CTX *sctx); | ^~~~~~~~~~~~~ cc1plus: all warnings being treated as errors
@kgpai I was trying to follow the guide to update the docker images for Circle CI to remove downloading and building folly as part of the image but I don't seem to have access to the f4d repository, do you know if this guide is still valid or is this repo internal? https://github.com/facebookincubator/velox/tree/main/.circleci#linux-testing
I was able to make the new circleci-container (it required a minor fix on setup-circleci.sh which I pushed on the PR) and I did update the .circleci/config.yml with the new local tag: I had to retag from prestocpp/velox-avx-circleci:raulcd-20220919 to raulcd:raulcd-20220919 to avoid CircleCI trying to retrieve it from remote. After all that I've been able to run the circleci local execute --job linux-build with the new image which has successfully built velox building folly from source:
$ circleci local execute --job linux-build
...
-- Building Folly from source
...
[1/1206] Building CXX object _deps/folly-build/CMakeFiles/folly_base.dir/folly/FollyMemcpy.cpp.o
[2/1206] Building CXX object _deps/folly-build/CMakeFiles/folly_base.dir/folly/FollyMemset.cpp.o
[3/1206] Building CXX object _deps/folly-build/CMakeFiles/folly_base.dir/folly/ClockGettimeWrappers.cpp.o
[4/1206] Building CXX object _deps/folly-build/CMakeFiles/folly_base.dir/folly/CancellationToken.cpp.o
[5/1206] Building CXX object _deps/folly-build/CMakeFiles/folly_base.dir/folly/executors/ExecutorWithPriority.cpp.o
[6/1206] Building CXX object _deps/folly-build/CMakeFiles/folly_base.dir/folly/Demangle.cpp.o
...
100% tests passed, 0 tests failed out of 141
Total Test time (real) = 515.13 sec
There was an out of memory locally with the fuzzer tests but I think this is due to local resources. I am marking the PR as ready for review as apart from pushing the new docker images and update the config.yml, I think this is now ready to be reviewed.
Hi Raul, the f4d repository is historical (that was what Velox was called internally before being opensourced). It no longer exists and the instructions have to be updated. I will create a task to do that.
I will work with you offline to give you docker credentials so you can update the remote image.
Thanks again @raulcd . Looks good ; I have still some questions around the macro flags - but I am going to start testing this on an M1 later today and if it looks good will start the merge process.
I've pushed a new docker image to be used on CI where folly is not installed as system dependency but there are a couple jobs that fail with a new Warning raised:
[329/1217] Building CXX object velox/common/base/tests/CMakeFiles/velox_base_test.dir/AsyncSourceTest.cpp.o
FAILED: velox/common/base/tests/CMakeFiles/velox_base_test.dir/AsyncSourceTest.cpp.o
ccache /opt/rh/gcc-toolset-9/root/bin/g++ -DBOOST_ALL_NO_LIB -DBOOST_ATOMIC_DYN_LINK -DBOOST_CONTEXT_DYN_LINK -DBOOST_DATE_TIME_DYN_LINK -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_REGEX_DYN_LINK -DBOOST_SYSTEM_DYN_LINK -DBOOST_THREAD_DYN_LINK -DFMT_LOCALE -DGFLAGS_IS_A_DLL=0 -I../../. -I../../velox/external/xxhash -I_deps/folly-src -I_deps/folly-build -I/usr/include/libdwarf -I../../third_party/xsimd/include -isystem ../../velox -isystem ../../velox/external -isystem ../../velox/external/duckdb -isystem ../../velox/external/duckdb/tpch/dbgen/include -isystem ../../third_party/googletest/googletest/include -isystem ../../third_party/googletest/googletest -mavx2 -mfma -mavx -mf16c -mlzcnt -std=c++17 -mbmi2 -D USE_VELOX_COMMON_BASE -D HAS_UNCAUGHT_EXCEPTIONS -Werror -Wall -Wextra -Wno-unused -Wno-unused-parameter -Wno-sign-compare -Wno-ignored-qualifiers -Wno-implicit-fallthrough -Wno-empty-body -Wno-class-memaccess -Wno-comment -Wno-int-in-bool-context -Wno-redundant-move -Wno-array-bounds -Wno-maybe-uninitialized -Wno-unused-result -Wno-format-overflow -Wno-strict-aliasing -Wno-type-limits -O3 -DNDEBUG -fPIE -Wno-nullability-completeness -Wno-deprecated-declarations -std=gnu++17 -MD -MT velox/common/base/tests/CMakeFiles/velox_base_test.dir/AsyncSourceTest.cpp.o -MF velox/common/base/tests/CMakeFiles/velox_base_test.dir/AsyncSourceTest.cpp.o.d -o velox/common/base/tests/CMakeFiles/velox_base_test.dir/AsyncSourceTest.cpp.o -c ../../velox/common/base/tests/AsyncSourceTest.cpp
In file included from _deps/folly-src/folly/Conv.h:128,
from _deps/folly-src/folly/Exception.h:25,
from _deps/folly-src/folly/detail/ThreadLocalDetail.h:30,
from _deps/folly-src/folly/ThreadLocal.h:52,
from _deps/folly-src/folly/executors/QueuedImmediateExecutor.h:22,
from ../.././velox/common/base/AsyncSource.h:20,
from ../../velox/common/base/tests/AsyncSourceTest.cpp:17:
_deps/folly-src/folly/lang/Pretty.h: In function 'void folly::futures::detail::coreDetachPromiseMaybeWithResult(folly::futures::detail::Core<T>&) [with T = folly::Unit]':
_deps/folly-src/folly/lang/Pretty.h:151:76: error: 'strlen' argument missing terminating nul [-Werror=stringop-overflow=]
I am curious on why this happens only on the linux-adapters and linux-benchmarks-basic but the linux-build job does not raise the warning using the same docker image. It fails on the fuzzer tests though, I'll keep investigating to fix CI.
@raulcd I suspect because linux-build builds against a smaller subset of velox. adapters and benchmarks build additional stuff that is not built by linux-build.
FYI - Testing on a mac m1, ran fine at first because of preinstalled folly. Deleted the /usr/local install of folly and now I see this :
-- Found Boost: /opt/homebrew/lib/cmake/Boost-1.79.0/BoostConfig.cmake (found suitable version "1.79.0", minimum required is "1.51.0") found components: context filesystem program_options regex system thread
-- Found gflags from package config /opt/homebrew/lib/cmake/gflags/gflags-config.cmake
-- Found libevent: /opt/homebrew/lib/libevent.dylib
CMake Error at /opt/homebrew/Cellar/cmake/3.24.2/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
Could NOT find OpenSSL, try to set the path to OpenSSL root folder in the
system variable OPENSSL_ROOT_DIR (missing: OPENSSL_CRYPTO_LIBRARY
OPENSSL_INCLUDE_DIR) (Required is at least version "1.1.1")
Call Stack (most recent call first):
/opt/homebrew/Cellar/cmake/3.24.2/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
/opt/homebrew/Cellar/cmake/3.24.2/share/cmake/Modules/FindOpenSSL.cmake:599 (find_package_handle_standard_args)
_build/debug/_deps/folly-src/CMake/folly-deps.cmake:81 (find_package)
_build/debug/_deps/folly-src/CMakeLists.txt:118 (include)
Do you know what value of OpenSSL root to set ? Its installed on my /opt/homebrew/opt/[email protected] , I tried setting it to that, and it isnt working..
** EDIT ** NVM was missing trailing / , and openssl problem is gone.
Now seeing the below:
FAILED: _deps/folly-build/CMakeFiles/folly_base.dir/folly/Fingerprint.cpp.o
ccache /Library/Developer/CommandLineTools/usr/bin/c++ -DBOOST_ATOMIC_DYN_LINK -DBOOST_ATOMIC_NO_LIB -DBOOST_CONTEXT_DYN_LINK -DBOOST_CONTEXT_NO_LIB -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_FILESYSTEM_NO_LIB -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_PROGRAM_OPTIONS_NO_LIB -DBOOST_REGEX_DYN_LINK -DBOOST_REGEX_NO_LIB -DBOOST_SYSTEM_DYN_LINK -DBOOST_SYSTEM_NO_LIB -DBOOST_THREAD_DYN_LINK -DBOOST_THREAD_NO_LIB -DFMT_LOCALE -DFOLLY_XLOG_STRIP_PREFIXES=\"/Users/kpai/src/velox:/Users/kpai/src/velox/_build/debug\" -DGFLAGS_IS_A_DLL=0 -D_GNU_SOURCE -D_REENTRANT -I/opt/homebrew/opt/icu4c/include -I/Users/kpai/src/velox/_build/debug/_deps/folly-src -I/Users/kpai/src/velox/_build/debug/_deps/folly-build -I/usr/local/include -I/opt/homebrew/opt/[email protected]/include -isystem /opt/homebrew/include -mcpu=apple-m1+crc -std=c++17 -fvisibility=hidden -D USE_VELOX_COMMON_BASE -D HAS_UNCAUGHT_EXCEPTIONS -Werror -Wall -Wextra -Wno-unused -Wno-unused-parameter -Wno-sign-compare -Wno-ignored-qualifiers -Wno-range-loop-analysis -Wno-mismatched-tags -g -g -Wall -Wextra -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk -fPIC -g -std=gnu++1z -finput-charset=UTF-8 -fsigned-char -Wall -Wno-deprecated -Wno-deprecated-declarations -Wno-sign-compare -Wno-unused -Wuninitialized -Wunused-label -Wunused-result -Wno-unused-parameter -Wno-overloaded-virtual -Wno-nullability-completeness -Wno-noexcept-type -Wno-inconsistent-missing-override -faligned-new -std=gnu++17 -MD -MT _deps/folly-build/CMakeFiles/folly_base.dir/folly/Fingerprint.cpp.o -MF _deps/folly-build/CMakeFiles/folly_base.dir/folly/Fingerprint.cpp.o.d -o _deps/folly-build/CMakeFiles/folly_base.dir/folly/Fingerprint.cpp.o -c /Users/kpai/src/velox/_build/debug/_deps/folly-src/folly/Fingerprint.cpp
In file included from /Users/kpai/src/velox/_build/debug/_deps/folly-src/folly/Fingerprint.cpp:17:
In file included from /Users/kpai/src/velox/_build/debug/_deps/folly-src/folly/Fingerprint.h:49:
In file included from /Users/kpai/src/velox/_build/debug/_deps/folly-src/folly/Range.h:22:
In file included from /Users/kpai/src/velox/_build/debug/_deps/folly-src/folly/Portability.h:21:
In file included from /Users/kpai/src/velox/_build/debug/_deps/folly-src/folly/CPortability.h:22:
In file included from /Users/kpai/src/velox/_build/debug/_deps/folly-src/folly/portability/Config.h:20:
/Users/kpai/src/velox/_build/debug/_deps/folly-build/folly/folly-config.h:22:34: error: 'TARGET_OS_SIMULATOR' is not defined, evaluates to 0 [-Werror,-Wundef-prefix=TARGET_OS_]
(TARGET_IPHONE_SIMULATOR || TARGET_OS_SIMULATOR || TARGET_OS_IPHONE))
^
/Users/kpai/src/velox/_build/debug/_deps/folly-build/folly/folly-config.h:22:57: error: 'TARGET_OS_IPHONE' is not defined, evaluates to 0 [-Werror,-Wundef-prefix=TARGET_OS_]
(TARGET_IPHONE_SIMULATOR || TARGET_OS_SIMULATOR || TARGET_OS_IPHONE))
'TARGET_OS_SIMULATOR' is not defined, evaluates to 0 [-Werror,-Wundef-prefix=TARGET_OS_]
I will try to investigate, I might be able to get access to an M1 that we use for testing purposes but it seems to be related to this folly issue: https://github.com/facebook/folly/issues/1673 and this commit: https://github.com/facebook/folly/commit/2fe2b638066face08aaa6fad6c896d89397e40b3
I will see if there's a way of avoiding this errors, could you try adding:
-Wno-undef-prefix to the list of FOLLY_CXX_FLAGS on the ThirdpartyToolchain.cmake file?
The flag can be used to ignore the warning as seen here: https://github.com/shirou/gopsutil/issues/976#issuecomment-973901185
@kgpai I have pushed a fix and have tested to build Velox on an AWS M1. I only had to define the OPENSSL_ROOT_DIR location:
export OPENSSL_ROOT_DIR=$(brew --prefix [email protected])
The fix ignores the previous warning that caused the failure and now builds successfully on M1. Please let me know if you can test this latest version and is successful. I also have pushed some fixes to fix the CircleCI builds using the newly generated docker image since your last review.
@raulcd Validated that the M1 build works after adding the undef-prefix flags. LGTM , going to start the merge process.
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@raulcd Going to merge this today.
Thanks! ping me if you see any issue, I'll be keeping an eye.
@raulcd Hi, I am trying to use Velox + Gluten develop Spark. After this commit, folly looks not installed in centos8 by default, but still installed in ubuntu code, so i could not use find_package(folly) in centos8 of gluten. I am curious about the reason of this difference between setup_ubuntu and set_centos8, should we have same affects after execute this scripts?
Hi @Yohahaha , Velox is moving towards hermetic builds, we are aiming to support dependencies installed on the system and also dynamically downloading and building if not present on system. This can be controlled using the <DEPENDENCY>_SOURCE flag. So for folly FOLLY_SOURCE = BUNDLED will result in folly being downloaded irrespective of whats on the system. There is no particular reason for not having folly in centos8 vs in ubunutu save that we are trying to test both modes and just choose arbitrarily. You should be free to install folly on centos should you choose.