Skip to content

[VL] Change to use Velox's wget_and_untar in setup-centos7.sh#9207

Merged
philo-he merged 1 commit intoapache:mainfrom
yaooqinn:wget
Apr 7, 2025
Merged

[VL] Change to use Velox's wget_and_untar in setup-centos7.sh#9207
philo-he merged 1 commit intoapache:mainfrom
yaooqinn:wget

Conversation

@yaooqinn
Copy link
Copy Markdown
Member

@yaooqinn yaooqinn commented Apr 2, 2025

What changes were proposed in this pull request?

This PR makes the dependency installation look up the local download source tar tall first.

We can reduce some network issues for developers while they build their own CI or just daily dev.

How was this patch tested?

tested locally

+ set -u
+ '[' -d /tmp/velox-deps ']'
+ run_and_time install_cmake
+ install_cmake
+ cd /tmp/velox-deps
+ wget_and_untar https://cmake.org/files/v3.28/ cmake-3.28.3.tar.gz cmake-3
+ local URL=https://cmake.org/files/v3.28/
+ local BIN=cmake-3.28.3.tar.gz
+ local DIR=cmake-3
+ mkdir -p cmake-3
+ [[ -f cmake-3.28.3.tar.gz ]]
+ tar -tzf cmake-3.28.3.tar.gz
+ echo 'Using cached cmake-3.28.3.tar.gz'
Using cached cmake-3.28.3.tar.gz
+ tar -xzf cmake-3.28.3.tar.gz -C cmake-3 --strip-components=1
+ return 0

@github-actions github-actions bot added the VELOX label Apr 2, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2025

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:

@yaooqinn yaooqinn marked this pull request as draft April 2, 2025 07:24
@yaooqinn yaooqinn marked this pull request as ready for review April 2, 2025 09:48
@@ -48,14 +48,29 @@ function yum_install {

function wget_and_untar {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yaooqinn, thanks for work!

I note upstream velox's wget_and_untar has a check for dir to let user decide whether to re-download the tar. Does it meet our needs?

Actually, by removing this wget_and_untar function, Velox's wget_and_untar (placed in setup-helper-functions.sh) will be used in this script. I am wondering whether this improvement can be put in upstream velox. Then, both setup-centos7.sh and setup-centos8.sh (maintained in Gluten) can use that, as well as setup-centos9.sh and setup-ubuntu.sh (maintained in Velox).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi, @philo-he. Thanks for the suggestion.

I had considered using velox's wget_and_untar, but I didn't apply it here.

  • Some of the dependency names might conflict as these names are in the form of vx.y.z.
  • velox's wget_and_untar does not check whether the tar ball is valid or not.

I am wondering whether this improvement can be put in upstream velox.

This is a good suggestion, I will try to send a PR to the upstream. But how about we focus the dependency management here first

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

On second thought, I agree w/ you about removing the wget_and_untar and use the upstream one

@philo-he philo-he changed the title Intall centos7 wget-able resources from local cache ahead [VL] Change to use Velox's wget_and_untar in setup-centos7.sh Apr 7, 2025
Copy link
Copy Markdown
Member

@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 for your continuous work!

@philo-he philo-he merged commit 55309a2 into apache:main Apr 7, 2025
48 checks passed
zhouyuan pushed a commit that referenced this pull request Apr 12, 2025
This issue was related to the recent pr #9207 which changes the script to use upstream Velox wget_and_untar function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants