Add setup functions for velox and build dependencies handled by package managers #8917

majetideepak · 2024-02-29T05:19:01Z

In the current setup scripts, the packages handled by the package manager are always installed
in MacOS, Centos8, and Ubuntu. This is unnecessary if we want to install a specific package.
Make these installs optional by wrapping them around a function.
Split the brew packages on MacOS based on the build vs Velox requirements.

netlify · 2024-02-29T05:19:19Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`a70d0a0`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6605ddac36575d0008be780a

assignUser

I like it! But from what I can tell you will also need to update the usage of the script across CI/dockerfiles right?

assignUser · 2024-03-01T03:44:02Z

scripts/setup-macos.sh

@@ -34,8 +34,8 @@ source $SCRIPTDIR/setup-helper-functions.sh
 NPROC=$(getconf _NPROCESSORS_ONLN)

 DEPENDENCY_DIR=${DEPENDENCY_DIR:-$(pwd)}
-MACOS_DEPS="ninja flex bison cmake ccache protobuf@21 icu4c boost gflags glog libevent lz4 lzo snappy xz zstd openssl libsodium"
-
+MACOS_VELOX_DEPS="flex bison protobuf@21 icu4c boost gflags glog libevent lz4 lzo snappy xz zstd openssl libsodium"


that protobuf will currently not be used due to our excat version matching

https://github.com/protocolbuffers/protobuf/releases/tag/v21.5
protobuf@21 is 3.21 which is our exact match?

I noticed it when building on macos that it wasn't used but I didn't look into it more

majetideepak · 2024-03-01T04:57:17Z

But from what I can tell you will also need to update the usage of the script across CI/dockerfiles right?

if [ "${INSTALL_PREREQUISITES:-Y}" == "Y" ];
The default is to install prerequisites. I don't think any changes are required in the current dockerfiles or CI scripts.

majetideepak · 2024-03-01T08:12:16Z

@assignUser, @kgpai Centos8 script ran here without any issues.
I tested macos changes locally and ubuntu changes on a VM.

assignUser · 2024-03-01T13:31:08Z

Ah I missed the default 👍

majetideepak · 2024-03-05T03:23:38Z

We will need to merge #8943 first since the Ubuntu setup script is not invoked correctly.

majetideepak · 2024-03-20T21:30:39Z

@assignUser, @kgpai Any comments on this PR? Thanks!

assignUser

Looks good, some minor comments.

assignUser · 2024-03-21T22:29:28Z

scripts/setup-macos.sh

@@ -34,8 +34,8 @@ source $SCRIPTDIR/setup-helper-functions.sh
 NPROC=$(getconf _NPROCESSORS_ONLN)

 DEPENDENCY_DIR=${DEPENDENCY_DIR:-$(pwd)}
-MACOS_DEPS="ninja flex bison cmake ccache protobuf@21 icu4c boost gflags glog libevent lz4 lzo snappy xz zstd openssl libsodium"
-
+MACOS_VELOX_DEPS="flex bison protobuf@21 icu4c boost gflags glog libevent lz4 lzo snappy xz zstd openssl libsodium"


I noticed it when building on macos that it wasn't used but I didn't look into it more

assignUser · 2024-03-21T22:31:36Z

scripts/setup-ubuntu.sh

+  # installing libunwind first fixes this.
+  ${SUDO} apt install -y libunwind-dev
+  ${SUDO} apt install -y \
+    g++ \


Suggested change

g++ \

build-essential \

might be more accurate?

scripts/setup-centos8.sh

assignUser · 2024-03-21T22:35:26Z

scripts/setup-centos8.sh

+  dnf config-manager --set-enabled powertools
+  dnf update -y
+  dnf_install ninja-build cmake curl ccache gcc-toolset-9 git wget which
+  dnf_install autoconf automake python3 libtool


Suggested change

dnf_install autoconf automake python3 libtool

dnf_install autoconf automake python39 python39-devel libtool

It will install 3.6 by default, -devel is for pyvelox

kgpai

Looks good, can you fix conflicts ?

kgpai · 2024-03-21T22:43:10Z

.github/workflows/benchmark.yml

@@ -88,7 +88,7 @@ jobs:

      - name: "Install dependencies"
        if: ${{ github.event_name == 'pull_request' }}
-        run: source velox/scripts/setup-ubuntu.sh
+        run: source velox/scripts/setup-ubuntu.sh && install_build_prerequisites &&  install_velox_deps_from_apt


nit: would it be nicer to pass an argument , say install to setup-ubuntu etc that then will run install_build_prerequisites and install_velox_deps_from_apt .

argument install is ambiguous since it does not specify what to install.
I can add another function install_apt_deps which includes the functions install_build_prerequisites and install_velox_deps_from_apt. How does that sound?
source velox/scripts/setup-ubuntu.sh && install_apt_deps

Thanks Deepak - that would also work

kgpai · 2024-03-21T23:00:08Z

scripts/setup-centos8.sh

  else
+    if [ "${INSTALL_PREREQUISITES:-Y}" == "Y" ]; then


You might want to add an else saying skipping installation of build prerequisites because of flag so and so..

majetideepak · 2024-03-22T01:01:41Z

I noticed it when building on macos that it wasn't used but I didn't look into it more

@assignUser what package is this?

assignUser · 2024-03-22T01:05:18Z

@majetideepak that was about protobuf@3 iirc but it has been a while so things might have changed both in the script and in brew ^^

assignUser · 2024-03-22T01:09:32Z

Depending on which PR lands first some additional changes will be needed here or in #8862 @kgpai

kgpai

Looks good, but I would like to ensure that we dont land with red signals from CI; I am hoping to land #9267 soon and hopefully you can rebase on top of that so that we get an all green build.

kgpai · 2024-03-28T03:37:35Z

.github/workflows/benchmark.yml

@@ -88,7 +88,7 @@ jobs:

      - name: "Install dependencies"
        if: ${{ github.event_name == 'pull_request' }}
-        run: source velox/scripts/setup-ubuntu.sh
+        run: source velox/scripts/setup-ubuntu.sh && install_build_prerequisites &&  install_velox_deps_from_apt


Thanks Deepak - that would also work

kgpai · 2024-03-28T03:42:03Z

scripts/setup-centos8.sh

+      echo "Skipping installation of build dependencies since INSTALL_PREREQUISITES is not set"
+    fi
+    # Activate gcc9; enable errors on unset variables afterwards.
+    source /opt/rh/gcc-toolset-9/enable || exit 1


Do we need to do this twice ?

One is in the if branch, and the other is in the else branch.

nit: maybe move it out then...

Can't move out. In the else branch, we can only enable this after installing build dependencies but before building the packages.

The if branch must be only taken if the build dependencies are already installed. The user is expecting to build specific packages.

…ge managers

kgpai · 2024-03-28T23:50:46Z

scripts/setup-centos8.sh

+      echo "Skipping installation of build dependencies since INSTALL_PREREQUISITES is not set"
+    fi
+    # Activate gcc9; enable errors on unset variables afterwards.
+    source /opt/rh/gcc-toolset-9/enable || exit 1


nit: maybe move it out then...

facebook-github-bot · 2024-03-28T23:52:03Z

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

facebook-github-bot · 2024-03-29T20:57:15Z

@kgpai merged this pull request in 8a1d225.

conbench-facebook · 2024-03-29T21:20:56Z

Conbench analyzed the 1 benchmark run on commit 8a1d2255.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…ge managers (facebookincubator#8917) Summary: In the current setup scripts, the packages handled by the package manager are always installed in Centos8 and Ubuntu. This is unnecessary if we want to install a specific package. Make these installs optional by wrapping them around a function. Split the brew packages on MacOS based on the build vs Velox requirements. Pull Request resolved: facebookincubator#8917 Reviewed By: xiaoxmeng Differential Revision: D55506473 Pulled By: kgpai fbshipit-source-id: 3e4fc31080faac6006dede1808ac9d76cb4b5f60

majetideepak requested review from kgpai and assignUser February 29, 2024 05:19

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 Feb 29, 2024

majetideepak marked this pull request as ready for review March 1, 2024 03:04

majetideepak force-pushed the add-function-system branch from 69a6581 to 52835dc Compare March 1, 2024 03:33

assignUser reviewed Mar 1, 2024

View reviewed changes

majetideepak force-pushed the add-function-system branch 2 times, most recently from 7cbbe0e to 788aa79 Compare March 1, 2024 07:09

majetideepak force-pushed the add-function-system branch 2 times, most recently from ea16d8f to 4ae3ada Compare March 2, 2024 09:07

majetideepak force-pushed the add-function-system branch 3 times, most recently from 7a92a1a to 973e06a Compare March 20, 2024 21:18

assignUser approved these changes Mar 21, 2024

View reviewed changes

kgpai approved these changes Mar 21, 2024

View reviewed changes

majetideepak force-pushed the add-function-system branch from 973e06a to e30f329 Compare March 22, 2024 01:00

majetideepak force-pushed the add-function-system branch from e30f329 to 6d6cb06 Compare March 22, 2024 10:44

kgpai reviewed Mar 28, 2024

View reviewed changes

majetideepak force-pushed the add-function-system branch 2 times, most recently from cb5cac0 to 11a49ab Compare March 28, 2024 04:09

Add setup functions for velox and build dependencies handled by packa…

a98d745

…ge managers

majetideepak force-pushed the add-function-system branch from 11a49ab to 863842e Compare March 28, 2024 18:22

add pip3 to centos8

a70d0a0

majetideepak force-pushed the add-function-system branch from 863842e to a70d0a0 Compare March 28, 2024 21:14

kgpai approved these changes Mar 28, 2024

View reviewed changes

facebook-github-bot closed this in 8a1d225 Mar 29, 2024

facebook-github-bot added the Merged label Mar 29, 2024

majetideepak deleted the add-function-system branch May 23, 2024 06:03

	dnf_install autoconf automake python3 libtool
	dnf_install autoconf automake python39 python39-devel libtool

Add setup functions for velox and build dependencies handled by package managers #8917

Add setup functions for velox and build dependencies handled by package managers #8917

Conversation

majetideepak commented Feb 29, 2024 • edited Loading

netlify bot commented Feb 29, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

assignUser 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

majetideepak commented Mar 1, 2024

majetideepak commented Mar 1, 2024

assignUser commented Mar 1, 2024

majetideepak commented Mar 5, 2024

majetideepak commented Mar 20, 2024

assignUser 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

kgpai 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

majetideepak commented Mar 22, 2024

assignUser commented Mar 22, 2024

assignUser commented Mar 22, 2024 • edited Loading

kgpai 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

majetideepak Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 28, 2024

facebook-github-bot commented Mar 29, 2024

conbench-facebook bot commented Mar 29, 2024

majetideepak commented Feb 29, 2024 •

edited

Loading

netlify bot commented Feb 29, 2024 •

edited

Loading

assignUser commented Mar 22, 2024 •

edited

Loading

majetideepak Mar 29, 2024 •

edited

Loading