[GLUTEN-11885][VL] Respect custom VELOX_HOME in build info generation#11905
[GLUTEN-11885][VL] Respect custom VELOX_HOME in build info generation#11905officialasishkumar wants to merge 2 commits intoapache:mainfrom
Conversation
|
Thank you for the fix |
| ;; | ||
| --velox_repo=*) | ||
| VELOX_REPO=("${arg#*=}") | ||
| VELOX_REPO="${arg#*=}" |
There was a problem hiding this comment.
Do we also need such modification for some other args?
There was a problem hiding this comment.
I checked the current script again, and I do not think we need the same change for the other args in this PR. The scalar handling here matters for --velox_repo, --velox_branch, and especially --velox_home because they are reused when building VELOX_PARAMETER and when propagating VELOX_HOME to child processes. The other flags are only consumed locally in this script today. If there is another arg that also needs to cross that boundary, I can handle it in a follow-up.
There was a problem hiding this comment.
@officialasishkumar, thanks for the check. Since this functionality is not well covered by CI, could you verify it in local build with a custom Velox home used? You may simply check a local resource file generated by gluten-build-info.sh.
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
I tried with these changes and specified a custom path for --velox_home, but after the build, it appears that the velox revision in the generated build info doesn't align with the revision of my custom velox home, according to gluten-core/target/generated-resources/gluten-build-info.properties. Could you help verify again? Thank you.
| return | ||
| fi | ||
|
|
||
| CACHED_VELOX_HOME=$(read_cmake_cache_path VELOX_HOME) |
There was a problem hiding this comment.
Is this detection always reliable? Can we just use this for getting velox home?
3 participants
What changes are proposed in this pull request?
Fixes #11885.
dev/gluten-build-info.shwas always reading Velox revision metadata from the defaultep/build-velox/build/velox_epcheckout, even when the build used a different Velox source tree via--velox_home.This PR updates the build-info path resolution so the Velox revision metadata is taken from the actual configured Velox checkout:
gluten-build-info.shVELOX_HOMEenvironment variable when presentcpp/build/CMakeCache.txtVELOX_HOMEentry before using the default pathVELOX_HOMEfromdev/builddeps-veloxbe.shso the one-shot Velox build flow propagates the configured path into Maven's build-info generation step--velox_repo,--velox_branch, and--velox_homeas scalars soVELOX_HOMEcan be exported to child processes correctlyHow was this patch tested?
bash -n dev/gluten-build-info.shbash -n dev/builddeps-veloxbe.sh/tmp/fake-veloxVELOX_HOME=/tmp/fake-velox bash dev/gluten-build-info.sh ... --backend veloxwrites the fake repo commit intogluten-build-info.propertiessource ./dev/builddeps-veloxbe.sh --velox_home=/tmp/fake-velox trueexportsVELOX_HOMEfor child processesWas this patch authored or co-authored using generative AI tooling?
No