Remove setup-circleci.sh and update setup scripts #8454

majetideepak · 2024-01-19T14:22:20Z

Move all adapter dependencies to setup-adapters.sh
Add DuckDB build to setup-centos8.sh

netlify · 2024-01-19T14:22:25Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`aaad1c0`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65b836ca3dd73b0008fd15f3

majetideepak · 2024-01-19T15:53:52Z

I will verify this on a VM and fix the adapter dependencies.

majetideepak · 2024-01-19T21:45:51Z

@assignUser can you please provide feedback? Thanks.

assignUser · 2024-01-23T04:51:34Z

Like the idea but there seem to be some issues with missing packages on the centos7 based image. Otherwise it looks good.

One caveat: we will likely get rid of the centos7 based images due to EOL, so it's probably not worth it to spend too much time on this.

majetideepak · 2024-01-23T15:41:38Z

Like the idea but there seem to be some issues with missing packages on the centos7 based image. Otherwise it looks good.

The issue is that we need to copy setup-centos8.sh in the dockerfile as well. Do you know where this dockerfile lives?

assignUser · 2024-01-24T13:23:51Z

In the scripts folder https://github.com/facebookincubator/velox/blob/main/scripts%2Fcircleci-container.dockfile

You will need to add the COPY in a separate PR to update the container and rebase here (or push the updated container to your fork and use it for testing here and revert before merge)

majetideepak · 2024-01-24T18:59:51Z

You will need to add the COPY in a separate PR to update the container

@assignUser The Docker images got built without having to COPY in a separate PR. Is there a way we can test the new image built from this PR? Can you also review again? I rebased with your fmt changes. Thanks.

majetideepak · 2024-01-24T19:03:56Z

I copied script-circleci.sh to script-centos8.sh except for the hadoop package that the CI tests need.

kgpai · 2024-01-24T21:11:20Z

Hi @majetideepak , Trying to understand motivation for this change, apart from refactoring and reducing code is there something else ? Would it make sense to have only the centos8 container say going forward (we should be off of circleci soon anyway) . Having one container with all dependencies for adapters etc, should speed things up imv .

majetideepak · 2024-01-24T22:22:44Z

@kgpai Currently, the Prestissimo docker build for centos8 is broken. The centos8 setup script diverged from circleci.sh script here https://github.com/facebookincubator/velox/pull/5743/files
On the prestissimo side this divergence caused an issue since folly, fbthrift are now installed in Velox in circleci.sh but not in centos8.sh
We should have one setup script for Centos8 in Velox. We should move all the adapter dependencies to setup-adapters.sh and use that to create docker containers.

assignUser · 2024-01-25T00:20:40Z

The Docker images got built without having to COPY in a separate PR. Is there a way we can test the new image built from this PR?

Yes you can build the change here but it will not be used as the PR job lacks permissions to upload the container. We can't test a freshly build container as the cci job starts within the container, so it has to be known before.

With gha we might be able to add a step that checks for changes in the container and rebuilds it if needed and uses that local container to run the actual build or so but that added complexity might not be worth it. I will think about it.

assignUser

We should have one setup script for Centos8 in Velox. We should move all the adapter dependencies to setup-adapters.sh

That makes sense, looking at these changes we can then just remove the separate cci script?

majetideepak · 2024-01-25T01:00:59Z

@assignUser Can you take a look now?
I need your help with the next steps. Let me know if I need to split this PR. Thanks.

kgpai · 2024-01-25T01:03:29Z

So to summarize:

There will be one centos8 build
This build will be used for all PR CI (except say macos etc)
It will have all dependencies (including the adapter ones, which should lead to faster builds).

Shall we also rename the container file from circleci-container to say just ci-container ?

majetideepak · 2024-01-25T01:08:55Z

Shall we also rename the container file from circleci-container to say just ci-container ?

This name makes sense, but I need help in landing this change. Let me know if I need to make this change in this PR or separately.

kgpai · 2024-01-25T01:13:58Z

@majetideepak Let me know how I can help.

We can make the name change in a separate PR if its easier.

assignUser · 2024-01-25T01:24:12Z

scripts/setup-centos8.sh

-wget_and_untar http://www.oberhumer.com/opensource/lzo/download/lzo-2.10.tar.gz lzo &
-wget_and_untar https://boostorg.jfrog.io/artifactory/main/release/1.72.0/source/boost_1_72_0.tar.gz boost &
-wget_and_untar https://github.com/google/snappy/archive/1.1.8.tar.gz snappy &
-wget_and_untar https://github.com/fmtlib/fmt/archive/10.1.1.tar.gz fmt &


This line went missing in the refactor and leading to the build errors due to no fmt source.

Thank you for pointing this out. Will fix this.

I copied the setup-circleci.sh to setup-centos8.sh this time again.

facebook-github-bot · 2024-01-29T22:43:54Z

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

scripts/setup-macos.sh

majetideepak · 2024-01-29T22:52:54Z

scripts/setup-macos.sh

  OPENSSL_ROOT_DIR=$(brew --prefix [email protected]) \
  cmake_install -DBUILD_TESTS=OFF -DFOLLY_HAVE_INT128_T=ON
 }

+function install_fizz {
+  github_checkout facebookincubator/fizz "${FB_OS_VERSION}"
+  OPENSSL_ROOT_DIR=$(brew --prefix [email protected]) \


@assignUser do we still need the OPENSSL_ROOT_DIR variable with the upgrade?

I tested this. We infact have to remove this variable. Fizz is failing to build otherwise.

[79/85] Linking CXX executable bin/fizz-bogoshim FAILED: bin/fizz-bogoshim : && /Library/Developer/CommandLineTools/usr/bin/c++ -mcpu=apple-m1+crc -std=c++17 -fvisibility=hidden -fvisibility=hidden -fvisibility-inlines-hidden -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk -mmacosx-version-min=13.6 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/BogoShim.dir/test/BogoShim.cpp.o -o bin/fizz-bogoshim -Wl,-rpath,/opt/homebrew/lib lib/libfizz.a /opt/homebrew/Cellar/libsodium/1.0.19/lib/libsodium.dylib /Users/deepak/workspace/oss/prestissimo_velox_deps/lib/libfolly.a /Users/deepak/workspace/oss/prestissimo_velox_deps/lib/libfmt.a /opt/homebrew/lib/libboost_context-mt.dylib /opt/homebrew/lib/libboost_filesystem-mt.dylib /opt/homebrew/lib/libboost_atomic-mt.dylib /opt/homebrew/lib/libboost_program_options-mt.dylib /opt/homebrew/lib/libboost_regex-mt.dylib /opt/homebrew/lib/libboost_system-mt.dylib /opt/homebrew/lib/libboost_thread-mt.dylib /opt/homebrew/lib/libevent.dylib /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/lib/libz.tbd /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/lib/libbz2.tbd /opt/homebrew/lib/liblz4.dylib /opt/homebrew/lib/libsnappy.dylib /opt/homebrew/lib/libsodium.dylib -lc++abi /opt/homebrew/opt/[email protected]/lib/libssl.dylib /opt/homebrew/opt/[email protected]/lib/libcrypto.dylib /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/lib/libz.tbd /opt/homebrew/lib/libzstd.dylib /opt/homebrew/lib/libglog.dylib /opt/homebrew/lib/libgflags.2.2.2.dylib /Users/deepak/workspace/oss/prestissimo_velox_deps/lib/libdouble-conversion.a && : ld: Undefined symbols: _EVP_PKEY_get_bits, referenced from: folly::AsyncSSLSocket::getSSLCertSize() const in libfolly.a[174](AsyncSSLSocket.cpp.o) _SSL_get1_peer_certificate, referenced from: folly::AsyncSSLSocket::getPeerCertificate() const in libfolly.a[174](AsyncSSLSocket.cpp.o) clang: error: linker command failed with exit code 1 (use -v to see invocation)

I was able to build this with openssl root dir etc..
The error you are seeing is because its picking up openssl 3 binary but building against openssl 1 header.. If it builds against openssl 1 binary you wont see that issue.

facebook-github-bot · 2024-01-29T23:43:43Z

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

facebook-github-bot · 2024-01-30T00:22:23Z

@kgpai merged this pull request in ec3b7a7.

conbench-facebook · 2024-01-30T00:44:20Z

Conbench analyzed the 1 benchmark run on commit ec3b7a70.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

mbasmanova · 2024-01-30T15:07:39Z

@kgpai @majetideepak @assignUser

Looks like this change add fizz, wangle and other Prestissimo-specific dependencies to Velox. I assume this is not intentional. Let's fix.

assignUser · 2024-01-30T15:14:51Z

These are needed for fbthrift, which is needed for remote functions (and in the future parquet when we switch Apache thrift for fbthrift)

mbasmanova · 2024-01-30T15:22:11Z

These are needed for fbthrift, which is needed for remote functions (and in the future parquet when we switch Apache thrift for fbthrift)

Interesting. So we have a lot of more dependencies in Velox now. Have we communicated addition of these dependencies somewhere?

majetideepak · 2024-01-30T15:57:29Z

So we have a lot of more dependencies in Velox now. Have we communicated addition of these dependencies somewhere?

@mbasmanova what we thought was a simple fmt upgrade that is backward compatible turned out to be false on MacOS.
This broke Prestissimo. This lead to other dependencies to be updated. I noticed we don't document dependencies in general.
I am pushing a PR shortly to add this documentation also add minimum required versions to CMake so that users know that the dependencies need an update.

majetideepak · 2024-01-30T16:00:57Z

Issue here #8602

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 Jan 19, 2024

majetideepak requested review from kgpai and pedroerp January 19, 2024 14:22

majetideepak marked this pull request as draft January 19, 2024 15:52

majetideepak force-pushed the fix-scripts branch from 42b518b to feac36d Compare January 19, 2024 19:35

majetideepak changed the title ~~Remove redundant setup-circleci.sh~~ Reuse setup-centos8.sh in setup-circlei.sh Jan 19, 2024

majetideepak marked this pull request as ready for review January 19, 2024 20:02

majetideepak requested a review from assignUser January 19, 2024 21:45

majetideepak force-pushed the fix-scripts branch from 9301565 to 3ae0d2c Compare January 24, 2024 17:53

assignUser reviewed Jan 25, 2024

View reviewed changes

majetideepak force-pushed the fix-scripts branch from 3ae0d2c to 66d89b6 Compare January 25, 2024 00:58

majetideepak changed the title ~~Reuse setup-centos8.sh in setup-circlei.sh~~ Remove setup-circleci.sh and update setup-centos8.sh Jan 25, 2024

assignUser reviewed Jan 25, 2024

View reviewed changes

majetideepak force-pushed the fix-scripts branch 2 times, most recently from 546543a to 7ea5e84 Compare January 25, 2024 03:48

majetideepak and others added 9 commits January 29, 2024 17:15

Remove setup-circleci.sh and update setup-centos8.sh

da0e612

cleanup

f39222a

use devtools set

e9add63

Add minio

ff95316

fix adapters.sh and add Minio build

4b28768

address review comments

6af0963

fix minio setup

0457ae8

Add fbthrift/mvfst/fizz/wangle and other dependencies to velox.

ffa0879

Fix MacOS command

36ec5a8

majetideepak force-pushed the fix-scripts branch from 4c7978f to 36ec5a8 Compare January 29, 2024 22:34

Remove folly fix

f0bb998

majetideepak commented Jan 29, 2024

View reviewed changes

scripts/setup-macos.sh Show resolved Hide resolved

majetideepak commented Jan 29, 2024

View reviewed changes

Fix MacOS setup script

aaad1c0

majetideepak changed the title ~~Remove setup-circleci.sh and update setup-centos8.sh~~ Remove setup-circleci.sh and update setup scripts Jan 29, 2024

facebook-github-bot closed this in ec3b7a7 Jan 30, 2024

facebook-github-bot added the Merged label Jan 30, 2024

majetideepak mentioned this pull request Jan 30, 2024

[native] Update build to support new dependency versions prestodb/presto#21797

Merged

majetideepak mentioned this pull request Jan 30, 2024

Add documentation for dependencies #8602

Closed

wanweitao mentioned this pull request Mar 15, 2024

[native] Update centos Dockerfile to use new setup script from velox prestodb/presto#22216

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove setup-circleci.sh and update setup scripts #8454

Remove setup-circleci.sh and update setup scripts #8454

majetideepak commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 19, 2024 •

edited

Loading

majetideepak commented Jan 19, 2024

majetideepak commented Jan 19, 2024

assignUser commented Jan 23, 2024

majetideepak commented Jan 23, 2024

assignUser commented Jan 24, 2024

majetideepak commented Jan 24, 2024

majetideepak commented Jan 24, 2024

kgpai commented Jan 24, 2024

majetideepak commented Jan 24, 2024 •

edited

Loading

assignUser commented Jan 25, 2024

assignUser left a comment

majetideepak commented Jan 25, 2024

kgpai commented Jan 25, 2024

majetideepak commented Jan 25, 2024

kgpai commented Jan 25, 2024 •

edited

Loading

assignUser Jan 25, 2024

majetideepak Jan 25, 2024

majetideepak Jan 25, 2024

facebook-github-bot commented Jan 29, 2024

majetideepak Jan 29, 2024

majetideepak Jan 29, 2024

kgpai Jan 29, 2024

facebook-github-bot commented Jan 29, 2024

facebook-github-bot commented Jan 30, 2024

conbench-facebook bot commented Jan 30, 2024

mbasmanova commented Jan 30, 2024

assignUser commented Jan 30, 2024

mbasmanova commented Jan 30, 2024

majetideepak commented Jan 30, 2024

majetideepak commented Jan 30, 2024

Remove setup-circleci.sh and update setup scripts #8454

Remove setup-circleci.sh and update setup scripts #8454

Conversation

majetideepak commented Jan 19, 2024 • edited Loading

netlify bot commented Jan 19, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

majetideepak commented Jan 19, 2024

majetideepak commented Jan 19, 2024

assignUser commented Jan 23, 2024

majetideepak commented Jan 23, 2024

assignUser commented Jan 24, 2024

majetideepak commented Jan 24, 2024

majetideepak commented Jan 24, 2024

kgpai commented Jan 24, 2024

majetideepak commented Jan 24, 2024 • edited Loading

assignUser commented Jan 25, 2024

assignUser left a comment

Choose a reason for hiding this comment

majetideepak commented Jan 25, 2024

kgpai commented Jan 25, 2024

majetideepak commented Jan 25, 2024

kgpai commented Jan 25, 2024 • edited Loading

assignUser Jan 25, 2024

Choose a reason for hiding this comment

majetideepak Jan 25, 2024

Choose a reason for hiding this comment

majetideepak Jan 25, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Jan 29, 2024

majetideepak Jan 29, 2024

Choose a reason for hiding this comment

majetideepak Jan 29, 2024

Choose a reason for hiding this comment

kgpai Jan 29, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Jan 29, 2024

facebook-github-bot commented Jan 30, 2024

conbench-facebook bot commented Jan 30, 2024

mbasmanova commented Jan 30, 2024

assignUser commented Jan 30, 2024

mbasmanova commented Jan 30, 2024

majetideepak commented Jan 30, 2024

majetideepak commented Jan 30, 2024

majetideepak commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 19, 2024 •

edited

Loading

majetideepak commented Jan 24, 2024 •

edited

Loading

kgpai commented Jan 25, 2024 •

edited

Loading