-
Notifications
You must be signed in to change notification settings - Fork 558
[GLUTEN-10155][INFRA] Fix build on macOS #10158
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
base: main
Are you sure you want to change the base?
Conversation
if [[ "$(uname)" == "Darwin" ]]; then | ||
clang_major_version=$(echo | clang -dM -E - | grep __clang_major__ | awk '{print $3}') | ||
if [ "${clang_major_version}" -ge 17 ]; then | ||
ARROW_WITH_ZLIB=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.
With ARROW_WITH_ZLIB=OFF, is there any impact on the functionalities?
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.
Here we just turn off on mac with clang 17, no impact on others.
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.
I meant its impact when clang 17 is used.
I note there are some fixes in Velox community for clang 17 build, e.g., facebookincubator/velox#13598. So I assume building velox/arrow can pass with clang 17 at that side. Curious if there is any other fix in Velox community instead of turning off this option.
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.
Velox can compile successfully probably because ZLIB_SOURCE=AUTO, I will try gluten later.
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.
There is no problem compiling arrow/velox using system's zlib, but compiling gluten cpp gives an error, it seems that the version is incompatible.
Undefined symbols for architecture arm64:
"_deflate", referenced from:
arrow::util::internal::(anonymous namespace)::GZipCodec::Compress(long long, unsigned char const*, long long, unsigned char*) in libarrow.a[120](compression_zlib.cc.o)
arrow::util::internal::(anonymous namespace)::GZipCompressor::Compress(long long, unsigned char const*, long long, unsigned char*) in libarrow.a[120](compression_zlib.cc.o)
arrow::util::internal::(anonymous namespace)::GZipCompressor::Flush(long long, unsigned char*) in libarrow.a[120](compression_zlib.cc.o)
I ran some UT locally on my Mac and there were no problems. I guess as long as don't use zlib compression, it shouldn't have any effect.
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.
If zlib could be disabled for mac, can it actually be disabled for all systems?
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 is risky. We just temporarily disable zlib and enable it after arrow is upgraded.
This is a zlib problem: madler/zlib#856 |
@@ -141,6 +141,7 @@ function cmake_install { | |||
} | |||
|
|||
function setup_macos { | |||
sed -i '' '/run_and_time install_arrow/d' 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.
Thanks for this fix!
dev/builddeps-veloxbe.sh
Outdated
if [[ "$(uname)" == "Darwin" ]]; then | ||
INSTALL_PREFIX=${INSTALL_PREFIX:-} | ||
else | ||
INSTALL_PREFIX=${INSTALL_PREFIX:-"/usr/local"} |
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.
The INSTALL_PREFIX could be set manually in .zshrc
for mac compilation. I wonder if we need this change to differentiate.
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.
It seems more appropriate to keep the default value of INSTALL_PREFIX consistent with velox's macOS.
What changes were proposed in this pull request?
(Fixes: #10155)
How was this patch tested?
local mac build