Refactor setup scripts #10670

czentgr · 2024-08-05T23:04:00Z

Use a common file to download and install external dependencies.
Extract versions for each library.

This also addresses #10860
xsimd is removed from brew and instead installed using the install function. The issue is caused by xsimd being newer than works for Velox at this point.

netlify · 2024-08-05T23:04:16Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`6a4dc12`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66d78fe53cc4c40008d4d3e1

majetideepak

@czentgr Looks great! Some comments.

majetideepak · 2024-08-08T17:45:29Z

README.md

 $ make
 ```

-Note that `setup-adapters.sh` supports MacOS and Ubuntu 20.04 or later.
+Note that the `install_adapters` command is available for the supports MacOS and


typo: supported

majetideepak · 2024-08-08T17:46:19Z

scripts/centos.dockerfile

@@ -16,6 +16,8 @@ ARG image=quay.io/centos/centos:stream9
 FROM $image

 COPY scripts/setup-helper-functions.sh /
+COPY scripts/setup-common.sh /
+COPY scripts/setup-linux.sh /


copy the versions file?

Yup. Adding it.

majetideepak · 2024-08-08T17:48:08Z

scripts/setup-linux.sh

+SCRIPTDIR=$(dirname "${BASH_SOURCE[0]}")
+source $SCRIPTDIR/setup-common.sh
+
+function install_hdfs {


Why is this not in common?

The macos version below does not have hadoop installed. Everything else is the same. Hadoop is required for testing. Let's add this hadoop install directly to ubuntu/centos install_hdfs and remove setup-linux.sh

czentgr · 2024-08-12T22:55:52Z

scripts/setup-centos9.sh

@@ -64,7 +60,7 @@ function install_velox_deps_from_dnf {
  dnf_install libevent-devel \
    openssl-devel re2-devel libzstd-devel lz4-devel double-conversion-devel \
    libdwarf-devel elfutils-libelf-devel curl-devel libicu-devel bison flex \
-    libsodium-devel zlib-devel
+    libsodium-devel zlib-devel gmock gtest


We need the gmock-devel here. This pulls in gtest-devel too.

Since #10422 we now properly detect system gtest, so having that in the images would be nice anyway.

Yeah. Actually, building the Arrow dependency failed for me from a fresh system until I had gtest-devel installed. Arrow has testing turned on...

On a side note: pip isn't used to install regex and cmake-format anymore? Was that removed?
On a developer machine this would be needed to run make format-check / make-format-fix. In the CI pipeline it has its own container for checking.
Do we want to add this?

See #10668 for the python changes, the CI container for the check job is mostly for clang-format I think because different versions can show very different results.

Right, the venv was added for MacOS. So far the python versions on Linux don't cause a problem but we likely will need to expand it. I was just wondering why we install clang-format/regex on MacOS but not the Linux platforms and whether or not we should have it commonly done.

Good point actually, not sure? They probably should? @majetideepak

We have a separate script for linux scripts/setup-check.sh. This was likely moved out to simplify building the check container.
Developers mainly use MacOS and they need the check set up. Linux is used for deployment and the checks are not required.
We can include setup-check.sh inside the linux scripts and install as well.
We need to ensure Linux and MacOS use the same clang-format version.

Linux is used for deployment and the checks are not required.

I think development on Linux is done. I suppose the developers did the setup themselves. There is an assumption that pip would install the same version on all platforms but it could be fixed.

Let me undo the change though and deal with this later.

Developers mainly use MacOS

eh, maybe (written on linux^^) but even then we want to make it easy to contribute so this should be included in the devsetup for linux as well

majetideepak

Changes look good. @kgpai Any thoughts on this?

assignUser · 2024-08-17T02:34:57Z

Could you add the macos setup script as a trigger to the macos workflow so it get's run with these changes as well?

Overall a long overdue refactor 👏 nice job!

czentgr · 2024-08-19T13:27:52Z

scripts/setup-macos.sh

+  run_and_time install_s3
+  run_and_time install_gcs
+  run_and_time install_abfs
+  run_and_time install_hdfs
 }

 function install_velox_deps {


@majetideepak Question: we are calling install_fmt but also install it through brew earlier. Should we pick one or the other? I see it was added a while back in one of your PRs.

Let's remove the brew install. I think fmt version should align with the other FB library versions.

Thanks will do. And now this has come up with boost. Brew keeps everything up to date and can cause issues on update. Now seen with boost 1.86.0 while Linux uses boost 1.84.0. Maybe for this we should also switch to the common install function?

Yes, lets use a common install function for boost as well.

assignUser · 2024-08-22T01:54:33Z

CI should go green if you rebase

.github/workflows/macos.yml

czentgr · 2024-08-22T21:09:47Z

.github/workflows/macos.yml

-          source scripts/setup-macos.sh
-          brew install $MACOS_BUILD_DEPS $MACOS_VELOX_DEPS
-
+          echo "OS used" ${{ matrix.os }}


TODO remove.

scripts/setup-centos9.sh

majetideepak

@czentgr Final set of comments.
Glad to see a bunch of duplication go away.
Thanks for working on this!

majetideepak · 2024-08-25T11:23:04Z

scripts/setup-common.sh

+  cmake_install re2 -DRE2_BUILD_TESTING=OFF
+}
+
+function install_gflags {


Do we need install_gflags and install_glog now that they are installed via the system package manager?

This is still built in Centos9 and doesn't come from the system install.
I checked and Centos9 has gflags 2.2.2 (from epel) which is the same version as it is currently built. Glog is also available as 0.3.5 but that is much outdated compared to 0.6.0 which is installed.
We could install gflags by the system and build glog.
The glog system install depends on gflags.

So I suppose we can move gflags to the system install and keep glog.

majetideepak · 2024-08-25T11:23:47Z

scripts/setup-common.sh

+
+function install_aws_deps {
+  local AWS_REPO_NAME="aws/aws-sdk-cpp"
+  local


remove empty local?

majetideepak · 2024-08-25T11:25:47Z

scripts/setup-common.sh

+}
+
+function install_minio {
+  local MINIO_ARCH=$1


nit: Maybe add defaults for macos arm (more devs use this platform).

majetideepak · 2024-08-25T11:30:33Z

scripts/setup-macos.sh

-function install_mvfst {
-  wget_and_untar https://github.com/facebook/mvfst/archive/refs/tags/${FB_OS_VERSION}.tar.gz mvfst
-  cmake_install mvfst -DBUILD_TESTS=OFF
+  local MINIO_ARCH=$MACHINE


MACHINE is defined in the setup-common.sh.
This if-else block here and inside other files can go to install_minio there.

czentgr · 2024-08-27T16:54:11Z

scripts/setup-macos.sh

-
-DEPENDENCY_DIR=${DEPENDENCY_DIR:-$(pwd)}
-MACOS_VELOX_DEPS="bison boost double-conversion flex fmt gflags glog googletest icu4c libevent libsodium lz4 lzo openssl protobuf@21 simdjson snappy thrift xz xsimd zstd"
+MACOS_VELOX_DEPS="bison double-conversion flex gflags glog googletest icu4c libevent libsodium lz4 lzo openssl protobuf@21 simdjson snappy thrift xz zstd"


Remove double-conversion. It is installed using the install function.

@majetideepak I tried to resolve this dual install.
The problem with removing it here is that this is needed for boost. And the CI pipleine has a path where the actual dependencies are not installed. Only the brew dependencies are installed and the rest comes from bundling.
double-conversion is not bundled and so if the setup script does not run the installation (remember we only turned it on for maOS 13) of the deps it'll fail.

So a couple of options:

Add it to be bundled

Remove install from being called and use version from brew (this just like with other dependencies means it can be updated at any time and cause issues).

Have the CI pipeline always run the setup script for macOS.

@assignUser What do you think?

We could run the install scripts and cache the dependencies, that would give the best control over versions and have the smallest impact on ci times.

How could we do that? It wouldn't be via container? And being on different versions of MacOS it isn't like we have a common build step that needs to be executed only once.

Or we can do that later and just get the scripts in for now and optimize later?

with the cache or stash action (though for this cache actually makes more sense) but yeah it's something we have to change in the workflow not the script, this PR is big enough as is we can do it as a follow up!

Let me then build the dependencies always. There is a current problem with fmt due to it being upgraded in homebrew (because ccache upgrade). We want to make changes to use the predefined and installed versions instead of using uncontrolled build deps from homebrew (which will always be found first because of the linkage). So we don't want to just get the macOS deps from homebrew. Lets see what happens.
This affects this PR as well.

Edit: using the bundled fmt will should work for now.

Use a common file to download and install external dependencies. Extract versions for each library.

czentgr · 2024-09-03T22:40:59Z

.github/workflows/macos.yml

@@ -78,7 +76,7 @@ jobs:

      - name: Configure Build
        env:
-          folly_SOURCE: BUNDLED #brew folly does not have int128
+          fmt_SOURCE: BUNDLED #picked up brew fmt (from ccache) is too new


Folly doesn't need to be force bundled because it is not coming from brew anymore but is installed by the setup script.
It uses fmt as bundled because the fmt version is too new. This need to be undone once the build script picks up the installed version via the setup script (and not the one present in homebrew from some dependency).

assignUser · 2024-09-04T02:22:35Z

I am not sure if this should be an addition to the scripts or the docs but it's easy to avoid system install of dependencies (see ftm version issue) by setting CMAKE_INSTALL_PREFIX=$VELOX_DEPS_PATH before running the script and CMAKE_{MODULE|PREFIX}_PATH=$VELOX_DEPS_PATH before the velox build to make the folder discoverable. (where VELOX_DEPS_PATH is the path you want to install the deps to)

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 Aug 5, 2024

czentgr requested review from majetideepak, assignUser and kgpai August 5, 2024 23:04

czentgr force-pushed the cz_refactor_setup_scripts branch from 764aae4 to 35b2e65 Compare August 6, 2024 17:30

majetideepak reviewed Aug 8, 2024

View reviewed changes

czentgr force-pushed the cz_refactor_setup_scripts branch from 35b2e65 to 8c44bdc Compare August 10, 2024 04:35

czentgr commented Aug 12, 2024

View reviewed changes

czentgr closed this Aug 13, 2024

czentgr force-pushed the cz_refactor_setup_scripts branch from 8c44bdc to 7474bae Compare August 13, 2024 18:43

czentgr reopened this Aug 13, 2024

czentgr force-pushed the cz_refactor_setup_scripts branch 2 times, most recently from a0eed92 to e80ea28 Compare August 13, 2024 22:19

majetideepak reviewed Aug 15, 2024

View reviewed changes

majetideepak marked this pull request as ready for review August 15, 2024 14:52

majetideepak changed the title ~~[WIP] Refactor setup scripts~~ Refactor setup scripts Aug 15, 2024

czentgr force-pushed the cz_refactor_setup_scripts branch 2 times, most recently from 94e6ace to 8d6d937 Compare August 15, 2024 18:26

czentgr commented Aug 19, 2024

View reviewed changes

czentgr force-pushed the cz_refactor_setup_scripts branch 3 times, most recently from 6eb7dfa to 33686b5 Compare August 20, 2024 21:06

majetideepak reviewed Aug 22, 2024

View reviewed changes

.github/workflows/macos.yml Show resolved Hide resolved

czentgr force-pushed the cz_refactor_setup_scripts branch from 33686b5 to 100065f Compare August 22, 2024 20:10

czentgr commented Aug 22, 2024

View reviewed changes

majetideepak reviewed Aug 22, 2024

View reviewed changes

scripts/setup-centos9.sh Outdated Show resolved Hide resolved

czentgr force-pushed the cz_refactor_setup_scripts branch from 100065f to ebc5cf6 Compare August 22, 2024 23:06

czentgr force-pushed the cz_refactor_setup_scripts branch from ebc5cf6 to c79ab70 Compare August 23, 2024 13:20

majetideepak reviewed Aug 25, 2024

View reviewed changes

czentgr force-pushed the cz_refactor_setup_scripts branch from c79ab70 to d5fe2be Compare August 26, 2024 14:07

majetideepak approved these changes Aug 26, 2024

View reviewed changes

czentgr force-pushed the cz_refactor_setup_scripts branch from d5fe2be to 8569270 Compare August 27, 2024 16:20

czentgr commented Aug 27, 2024

View reviewed changes

czentgr force-pushed the cz_refactor_setup_scripts branch from 8569270 to d0e9c1f Compare August 29, 2024 15:08

Refactor setup scripts

6a4dc12

Use a common file to download and install external dependencies. Extract versions for each library.

czentgr force-pushed the cz_refactor_setup_scripts branch from d0e9c1f to 6a4dc12 Compare September 3, 2024 22:38

czentgr commented Sep 3, 2024

View reviewed changes

assignUser mentioned this pull request Sep 4, 2024

Fix fmt V11 build errors #10904

Closed

yingsu00 mentioned this pull request Sep 4, 2024

Fmt folly update #10924

Closed

majetideepak mentioned this pull request Sep 23, 2024

Fallback install tests are not covered in CI #11057

Closed

assignUser mentioned this pull request Sep 24, 2024

Add lib stemmer in setup scripts #10984

Closed

Refactor setup scripts #10670

Are you sure you want to change the base?

Refactor setup scripts #10670

Conversation

czentgr commented Aug 5, 2024 • edited Loading

netlify bot commented Aug 5, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assignUser Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

assignUser Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

assignUser commented Aug 17, 2024

Choose a reason for hiding this comment

majetideepak Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assignUser commented Aug 22, 2024

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assignUser commented Sep 4, 2024 • edited Loading

czentgr commented Aug 5, 2024 •

edited

Loading

netlify bot commented Aug 5, 2024 •

edited

Loading

assignUser Aug 13, 2024 •

edited

Loading

czentgr Aug 15, 2024 •

edited

Loading

assignUser Aug 17, 2024 •

edited

Loading

majetideepak Aug 20, 2024 •

edited

Loading

czentgr Sep 3, 2024 •

edited

Loading

assignUser commented Sep 4, 2024 •

edited

Loading