-
Notifications
You must be signed in to change notification settings - Fork 447
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
[VL] Fix for centos9 build of Gluten #6183
Conversation
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/apache/incubator-gluten/issues Then could you also rename commit message and pull request title in the following format?
See also: |
Run Gluten Clickhouse CI |
@FelixYBW - I am able to build Gluten on Centos9 with these changes. Have got the jar generated, now have to consume the jar and test it.
|
@zhztheplayer - Could you please check on this PR if there is some pending action from me ? |
centos 9 or centos stream 9? |
@wanglinsong - Yes, I am using |
@FelixYBW - Could you please help reviewing ? |
@deepashreeraghu I am hitting the following warning which is being handled as an error. Are you not seeing this?
|
ep/build-velox/src/get_velox.sh
Outdated
# make this function Reentrant | ||
git checkout scripts/setup-centos9.sh | ||
# need set BUILD_SHARED_LIBS flag for thrift | ||
#sed -i "/cd fbthrift/{n;s/cmake_install -Denable_tests=OFF/cmake_install -Denable_tests=OFF -DBUILD_SHARED_LIBS=OFF/;}" scripts/setup-centos9.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.
remove commented command?
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.
Updated
@yma11 How was this fixed on the Gluten side? |
Looks like we need this fix #5686 for Centos Stream 9 + GCC11 as well. |
It doesn't pop in most of our environments so we don't handle this formally at Gluten side yet. Haven't found a fix better than the workaround by adding flag |
@yma11 thanks. I am fixing this on the Velox side: https://github.com/facebookincubator/velox/pull/10469/files |
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 also verified that this PR works for Centos9 with one fix facebookincubator/velox#10469 on the Velox side.
I see that there are conflicts for this branch. |
c236d73
to
dbc4b8c
Compare
dbc4b8c
to
2d78033
Compare
2d78033
to
e278d5d
Compare
@majetideepak - Thanks for reviewing. I have rebased. Now I am rebuilding with latest changes and will retest it to confirm |
@majetideepak - I am able to build with latest set of changes too. Now using the built Gluten jar for further testing. |
@majetideepak - I have tested this and basic tests are going through fine with latest build. |
ep/build-velox/src/get_velox.sh
Outdated
sed -i '/^ dnf_install ninja-build/a\ dnf_install yasm\' scripts/setup-centos9.sh | ||
fi | ||
if [[ $ENABLE_HDFS == "ON" ]]; then | ||
sed -i '/cd protobuf/{n;s/\.\/configure --prefix=\/usr/\.\/configure CXXFLAGS="-fPIC" --prefix=\/usr\/local/;}' scripts/setup-centos9.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.
Based on process_setup_centos8
, should this be above and not under if [[ $ENABLE_HDFS == "ON" ]];
?
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.
Updated it to be like centos8
@FelixYBW, @zhztheplayer Can you help review this PR? Thanks! |
@yma11 can you help review this PR as well? Thanks! |
@deepashreeraghu can you add some code to ensure the |
@yma11 - Updated and verified it too. Please review. |
Thanks! @yma11 |
@deepashreeraghu, thanks so much for your work! We have a weekly build job to verify no build issue on some mainstream OS. Could you add centos-9 for tracking? |
@PHILO-HE - Sure, will do . I will create a separate PR for that. Updating this branch will be a hassle. Hope it is ok ? |
@deepashreeraghu, yes, a separate PR. Thanks! |
What changes were proposed in this pull request?
TO support building Gluten on Centos9
How was this patch tested?
I am trying to build Gluten on Centos9. Will update here once I am able to build successfully.