Skip to content
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] enable VCPKG build by default #7131

Closed
wants to merge 12 commits into from

Conversation

zhouyuan
Copy link
Contributor

@zhouyuan zhouyuan commented Sep 5, 2024

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

(Fixes: #ISSUE-ID)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

@github-actions github-actions bot added the BUILD label Sep 5, 2024
Copy link

github-actions bot commented Sep 5, 2024

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?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@github-actions github-actions bot added INFRA and removed INFRA labels Sep 5, 2024
@PHILO-HE
Copy link
Contributor

PHILO-HE commented Sep 6, 2024

@zhouyuan, just tested on my side. It works on ubuntu-20.04.

Maybe, we can keep one CI job using dynamic link by disabling vcpkg here.
https://github.com/apache/incubator-gluten/blob/main/.github/workflows/velox_backend.yml#L1012

@@ -173,6 +173,8 @@ if [ "$ENABLE_VCPKG" = "ON" ]; then
# vcpkg will install static depends and init build environment
envs="$("$GLUTEN_DIR/dev/vcpkg/init.sh")"
eval "$envs"
else
echo "Dynamic linked libs based building is deprecated, please enabled VCPKG based building."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we reserve dynamic link? I found static build is unstable in CI, it's better keep both way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dynamic link will be kept but we need to print one message. The static packaging has been used starts from gluten 1.0 and seems working well
The issue on dynamic packaging is on the dependencies update case(especially for the minor version update on libraries) , we need to update the related loader as well

https://github.com/apache/incubator-gluten/blob/main/backends-velox/src/main/scala/org/apache/gluten/utils/SharedLibraryLoaderUbuntu2204.scala

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static packaging has been used starts from gluten 1.0 and seems working well

curious how many users use static packaging directly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, most of our customers have already use static link in their production ready environment.
One of the major reason is our 3rd party package script is no longer to be updated.
Only a few customers are still using dynamic link since they maintain their own 3rd party libraries instead to use the script in Gluten.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7115 (comment)

Is the issue caused by static link?

@github-actions github-actions bot added the VELOX label Sep 13, 2024
Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>
Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>
This reverts commit 70fea33.

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>
Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>
Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>
Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>
Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>
@zhouyuan zhouyuan force-pushed the wip_vcpkg_build_default branch from 1645565 to 8a0e54e Compare September 13, 2024 09:17
@@ -1,6 +1,11 @@
#!/bin/bash

BASEDIR=$(dirname $0)

if [ `uname -m` = "x86_64" ]; then
sudo -E ${BASEDIR}/setup-build-depends.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path may be sudo -E ${BASEDIR}/vcpkg/setup-build-depends.sh ?

Copy link

github-actions bot commented Dec 1, 2024

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale stale label Dec 1, 2024
@github-actions github-actions bot removed the stale stale label Dec 12, 2024
@FelixYBW
Copy link
Contributor

@zhouyuan Let's close this since we can't get an agreement.

@Yohahaha currently one CI uses the dynamic link. You may run some jekins job internally to track the dynamic one.

@FelixYBW FelixYBW closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants