-
Notifications
You must be signed in to change notification settings - Fork 81
Fix velox dylib loading paths for conda build on macos #355
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| #!/usr/bin/env bash | ||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| # All rights reserved. | ||
| # | ||
| # This source code is licensed under the BSD-style license found in the | ||
| # LICENSE file in the root directory of this source tree. | ||
|
|
||
| function fix_velox_dylib_paths() { | ||
| velox_so=$1 | ||
|
|
||
| # other libs | ||
| libs="libgflags libglog libz libssl libcrypto libbz2 liblz4 libzstd libsodium" | ||
|
|
||
| for libx in ${libs} | ||
| do | ||
| liby=$(otool -L "${velox_so}" | sed -n -e "s/\\(.*${libx}\..*dylib\\).*/\\1/p" | tr -d '[:blank:]') | ||
|
|
||
| install_name_tool -change "${liby}" "@loader_path/../../../${libx}.dylib" "${velox_so}" | ||
| done | ||
|
|
||
| libx=libevent | ||
| # libevent | ||
| liby=$(otool -L "${velox_so}" | sed -n -e "s/\\(.*${libx}-.*dylib\\).*/\\1/p" | tr -d '[:blank:]') | ||
| install_name_tool -change "${liby}" "@loader_path/../../../${libx}.dylib" "${velox_so}" | ||
|
|
||
| # boost libs | ||
| boost_libs="libboost_context libboost_filesystem libboost_atomic libboost_regex libboost_system libboost_thread" | ||
|
|
||
| for libx in ${boost_libs} | ||
| do | ||
| liby=$(otool -L "${velox_so}" | sed -n -e "s/\\(.*${libx}.*dylib\\).*/\\1/p" | tr -d '[:blank:]') | ||
|
|
||
| install_name_tool -change "${liby}" "@loader_path/../../../${libx}.dylib" "${velox_so}" | ||
| done | ||
|
|
||
| libx=libboost_program | ||
| liby=$(otool -L "${velox_so}" | sed -n -e "s/\\(.*${libx}.*dylib\\).*/\\1/p" | tr -d '[:blank:]') | ||
| install_name_tool -change "${liby}" "@loader_path/../../../${libx}_options.dylib" "${velox_so}" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,20 +20,22 @@ requirements: | |
| - typing | ||
| - tabulate | ||
| - pyarrow | ||
| - arrow | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right , or
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and |
||
| - arrow-cpp==8.0.0 | ||
| - cffi | ||
| - glog==0.4.0 | ||
| - glog==0.6.0 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. anything changed on Velox side?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think velox always uses the glog installed from brew, which was recently updated from 0.5.0 to 0.6.0. Building with glog0.6.0 and running with glog0.4.0 will not work (some symbol not found error when running). However, the conda default channel hasn't updated to 0.6.0 yet, it only exists on conda-forge, hence this change.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also why we need to specify |
||
| - libsodium | ||
| {{ environ.get('CONDA_PYTORCH_CONSTRAINT') }} | ||
| - libboost | ||
| - libevent | ||
| - bzip2 | ||
| - lz4-c | ||
|
|
||
| build: | ||
| string: py{{py}} | ||
| script_env: | ||
| - BUILD_VERSION | ||
| - CPU_TARGET | ||
| - PYTHON_VERSION | ||
|
|
||
| #GitHub Actions usually provide Mac without AVX2 support, for now just skip the test until Velox can be run without AVX2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so the test is already enabled now, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, that's why I'm removing the stale comment here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might need to add command
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah here I'm only running an import test for now. For some reason the last time I ran the tests, there were some ones failing on some path comparison issues (don't remember the details, but something like comparing './abc' to 'abc failed). This will go into another PR to investigate. |
||
| #pytest --no-header -v torcharrow/test | ||
| test: | ||
| imports: | ||
| - torcharrow | ||
|
|
||
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.
nit: can we check there is exactly one parameter ?
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.
can you elaborate?