-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Setup scripts #12321
base: main
Are you sure you want to change the base?
refactor: Setup scripts #12321
Conversation
✅ Deploy Preview for meta-velox canceled.
|
449b3a0
to
b5d0ca3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for continuing the work on my original PR.
I should get co-authorship on this.
Please also note you have build failures. Please investigate.
@@ -67,7 +67,7 @@ jobs: | |||
env: | |||
HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK: "TRUE" | |||
run: | | |||
source scripts/setup-macos.sh | |||
scripts/setup-macos.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will run the full script and not just the functions below. The purpose of this was to get most dependencies from the bundle and not rely on the system installation of the dependencies. You can see the functions below only run a subset.
Please read the comment from @majetideepak in my original PR about this. We need more changes to refactor this build script or we leave the original intent and separate the bundling vs non-bundling between the versions of macOS later.
source $SCRIPTDIR/setup-helper-functions.sh | ||
source $SCRIPTDIR/setup-versions.sh | ||
|
||
NPROC=$(getconf _NPROCESSORS_ONLN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from the setup-helper-functions sourcing.
|
||
function install_duckdb { | ||
if $BUILD_DUCKDB ; then | ||
echo 'Building DuckDB' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed.
|
||
# install azure-storage-files-datalake | ||
cmake_install sdk/storage/azure-storage-files-datalake -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DBUILD_SHARED_LIBS=OFF | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs newline.
@@ -30,24 +30,18 @@ set -x # Print commands that are executed. | |||
|
|||
SCRIPTDIR=$(dirname "${BASH_SOURCE[0]}") | |||
export INSTALL_PREFIX=${INSTALL_PREFIX:-"$(pwd)/deps-install"} | |||
source $SCRIPTDIR/setup-helper-functions.sh | |||
source $SCRIPTDIR/setup-common.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overrides the INSTALL_PREFIX from the previous line to possibly the default /usr/local
.
INSTALL_PREFIX is likely something that should be set in each OS separately although the Linux ones share the same location.
MACOS_VELOX_DEPS="bison flex gflags glog googletest icu4c libevent libsodium lz4 lzo openssl protobuf@21 simdjson snappy thrift xz zstd" | ||
MACOS_BUILD_DEPS="ninja cmake ccache" | ||
|
||
SUDO="${SUDO:-""}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
NLOHMAN_JSON_VERSION="v3.11.3" | ||
GOOGLE_CLOUD_CPP_VERSION="v2.22.0" | ||
HADOOP_VERSION="2.10.1" | ||
AZURE_SDK_VERSION="12.8.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line needed
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.