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] Improve package scripts #6569

Merged
merged 2 commits into from
Jul 29, 2024
Merged

[VL] Improve package scripts #6569

merged 2 commits into from
Jul 29, 2024

Conversation

wForget
Copy link
Member

@wForget wForget commented Jul 24, 2024

What changes were proposed in this pull request?

Improve package scripts:

  • Extract build-thirdparty.sh script
  • Set PROMPT_ALWAYS_RESPOND=y for package scripts
  • Pass package arguments to buildbundle-veloxbe.sh
  • Add build thirdparty for package-vcpkg.sh

How was this patch tested?

Manual package test

Copy link

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:

@FelixYBW
Copy link
Contributor

@weiting-chen Do we still need the package.sh if we switch to static build? I never used the .sh.

@wForget
Copy link
Member Author

wForget commented Jul 26, 2024

if we switch to static build?

@FelixYBW Does static build mean using vcpkg?

@FelixYBW
Copy link
Contributor

@FelixYBW Does static build mean using vcpkg?

Yes.

@wForget
Copy link
Member Author

wForget commented Jul 26, 2024

@FelixYBW Does static build mean using vcpkg?

Yes.

Got it, thanks. I also prefer this packaging way. This PR also adds build thirdparty for package-vcpkg.sh. Could you help review it?

@FelixYBW FelixYBW requested a review from PHILO-HE July 26, 2024 05:50
./dev/buildbundle-veloxbe.sh --build_tests=ON --build_benchmarks=ON --enable_s3=ON --enable_hdfs=ON "$@"

# make thirdparty package
./dev/build-thirdparty.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 script is for vcpkg build with all dependencies statically linked. No need to pack shared libs.

Copy link
Member Author

Choose a reason for hiding this comment

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

This script is for vcpkg build with all dependencies statically linked. No need to pack shared libs.

Sorry, I didn't look at the document carefully, does it mean we no longer need the thirdparty lib jar when using package-vcpkg.sh? I will try it, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wForget, yes, third-party jar is useless when vcpkg build is enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, I will close this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Extract build-thirdparty.sh script
Set PROMPT_ALWAYS_RESPOND=y for package scripts
Pass package arguments to buildbundle-veloxbe.sh

Are these improvements reasonable? If so, I will continue working

Copy link
Contributor

Choose a reason for hiding this comment

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

@wForget , yes, you can just keep these changes. Thanks!

@wForget wForget requested a review from PHILO-HE July 29, 2024 02:50
Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

Thanks!

@PHILO-HE PHILO-HE merged commit 03765af into apache:main Jul 29, 2024
39 checks passed
@PHILO-HE
Copy link
Contributor

PHILO-HE commented Aug 5, 2024

@wForget, the weekly build job failed, seems related to this pr. Could you help take a look?
https://github.com/apache/incubator-gluten/actions/runs/10238892265/job/28323839510

@wForget
Copy link
Member Author

wForget commented Aug 6, 2024

@wForget, the weekly build job failed, seems related to this pr. Could you help take a look? https://github.com/apache/incubator-gluten/actions/runs/10238892265/job/28323839510

Sorry, this PR unexpectedly introduced the following code. I will delete it.

https://github.com/wForget/gluten/blob/671f874aa0557fc0a6632e65667dce9a25eafeea/dev/package.sh#L12-L18

weiting-chen pushed a commit to weiting-chen/gluten that referenced this pull request Nov 18, 2024
weiting-chen added a commit that referenced this pull request Nov 20, 2024
* [MINOR] Remove duplicate mvn packages (#6560)

* [VL] Improve package scripts (#6569)

* [GLUTEN-6569][VL] Delete unnecessary gcc9 enabling from package script (#6730)

* [GLUTEN-7118][VL] Fix duckdb target issue when vcpkg is enabled (#7117)

---------

Co-authored-by: Zhen Wang <643348094@qq.com>
Co-authored-by: PHILO-HE <feilong.he@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants