Skip to content

build: Avoid staying in subdir of dependency after it has been installed#15113

Closed
Zouxxyy wants to merge 1 commit intofacebookincubator:mainfrom
Zouxxyy:dev/update-sh
Closed

build: Avoid staying in subdir of dependency after it has been installed#15113
Zouxxyy wants to merge 1 commit intofacebookincubator:mainfrom
Zouxxyy:dev/update-sh

Conversation

@Zouxxyy
Copy link
Contributor

@Zouxxyy Zouxxyy commented Oct 10, 2025

In apache/gluten#10836, we encountered a Velox compilation issue caused by the changes introduced in #14849. The error occurred because ABSOLUTE_SCRIPTDIR was modified.

2025-10-09T03:42:02.6189401Z ++ realpath scripts
2025-10-09T03:42:02.6207867Z + ABSOLUTE_SCRIPTDIR=/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts
2025-10-09T03:42:02.6210432Z + VELOX_ARROW_CMAKE_PATCH=/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch
2025-10-09T03:42:02.6212114Z + cd /__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/arrow
2025-10-09T03:42:02.6213956Z + git apply /__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch
2025-10-09T03:42:02.6230732Z error: can't open patch '/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch': No such file or directory
2025-10-09T03:42:02.6235097Z + exit 1

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2025
@netlify
Copy link

netlify bot commented Oct 10, 2025 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4e3b611
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/68f516d9bb260d000813ab05

Zouxxyy
Zouxxyy commented Oct 10, 2025
View reviewed changes
scripts/setup-common.sh Outdated
# shellcheck source-path=SCRIPT_DIR

SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}")
ABSOLUTE_SCRIPTDIR=$(realpath "$SCRIPT_DIR")
Copy link
Contributor Author

@Zouxxyy Zouxxyy Oct 10, 2025

Choose a reason for hiding this comment

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

Maybe I can directly replace SCRIPT_DIR with ABSOLUTE_SCRIPTDIR?

@PHILO-HE
Copy link
Contributor

PHILO-HE commented Oct 10, 2025

@Zouxxyy, could you add some essential description explaining why this issue occurred? I assume this PR is not just for Gluten Velox build. Thanks.

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Oct 10, 2025 •
edited
Loading

@Zouxxyy, could you add some essential description explaining why this issue occurred? I assume this PR is not just for Gluten Velox build. Thanks.

here is log in https://github.com/apache/incubator-gluten/actions/runs/18381728104/job/52369538057?pr=10836

SCRIPT_DIR is a relative directory, which in the logs shows as SCRIPT_DIR=scripts. And, in the command github_checkout abseil/abseil-cpp "${ABSEIL_VERSION}" --depth 1, it executes cd abseil-cpp, causing the subsequent use of ABSOLUTE_SCRIPTDIR to become problematic.

2025-10-09T03:07:59.5347886Z + source scripts/setup-common.sh
2025-10-09T03:07:59.5351159Z +++ dirname scripts/setup-common.sh
2025-10-09T03:07:59.5365967Z ++ SCRIPT_DIR=scripts


2025-10-09T03:15:47.0112286Z + git clone -q -b 20240116.2 --depth 1 https://github.com/abseil/abseil-cpp.git
2025-10-09T03:15:47.6094508Z + cd abseil-cpp


2025-10-09T03:42:02.6207867Z + ABSOLUTE_SCRIPTDIR=/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts
2025-10-09T03:42:02.6210432Z + VELOX_ARROW_CMAKE_PATCH=/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch
2025-10-09T03:42:02.6212114Z + cd /__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/arrow
2025-10-09T03:42:02.6213956Z + git apply /__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch
2025-10-09T03:42:02.6230732Z error: can't open patch '/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch': No such file or directory
2025-10-09T03:42:02.6235097Z + exit 1

I think there are two things we could consider—what do you think? @PHILO-HE

  • Internally add popd in github_checkout and use cmake_install_dir instead of cmake_install.
  • Use an absolute path for SCRIPT_DIR.

@assignUser
Copy link
Collaborator

assignUser commented Oct 11, 2025

Internally add popd in github_checkout and use cmake_install_dir instead of cmake_install.

Yes, please implement this change (across all setup scripts!). We shouldn't stay in the subdir for the dependency after it has been installed. We must be nesting these dirs deeply? Interesting that it never lead to issue previously.

@Zouxxyy Zouxxyy changed the title build: Use the correct ABSOLUTE_SCRIPTDIR in setup-common.sh build: Avoid staying in subdir of dependency after it has been installed Oct 11, 2025
@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Oct 11, 2025

Yes, please implement this change (across all setup scripts!). We shouldn't stay in the subdir for the dependency after it has been installed. We must be nesting these dirs deeply? Interesting that it never lead to issue previously.

I've pushed the changes, but not sure where I can check if the CI is passing.

@assignUser
Copy link
Collaborator

assignUser commented Oct 12, 2025

I've pushed the changes, but not sure where I can check if the CI is passing.

Because this is your first contribution I have to manually run ci :) should be going now!

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Oct 13, 2025

@assignUser The previous commit had a formatting issue, I fix it, could you help re-trigger the CI, Thanks.

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Oct 14, 2025

@assignUser The faild ci may fixed by #15040, I rebased, could you help re-trigger the CI, Thanks.

@assignUser assignUser added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Oct 19, 2025
@meta-codesync
Copy link

meta-codesync bot commented Oct 20, 2025

@pedroerp has imported this pull request. If you are a Meta employee, you can view this in D85060996.

@meta-codesync meta-codesync bot closed this in a52307a Oct 20, 2025
@meta-codesync
Copy link

meta-codesync bot commented Oct 20, 2025

@pedroerp merged this pull request in a52307a.

mhaseeb123 pushed a commit to mhaseeb123/velox that referenced this pull request Oct 27, 2025
@Zouxxyy @mhaseeb123
build: Avoid staying in subdir of dependency after it has been instal…
1001b57 
…led (facebookincubator#15113)

Summary:
In apache/gluten#10836, we encountered a Velox compilation issue caused by the changes introduced in facebookincubator#14849. The error occurred because ABSOLUTE_SCRIPTDIR was modified.

```
2025-10-09T03:42:02.6189401Z ++ realpath scripts
2025-10-09T03:42:02.6207867Z + ABSOLUTE_SCRIPTDIR=/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts
2025-10-09T03:42:02.6210432Z + VELOX_ARROW_CMAKE_PATCH=/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch
2025-10-09T03:42:02.6212114Z + cd /__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/arrow
2025-10-09T03:42:02.6213956Z + git apply /__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch
2025-10-09T03:42:02.6230732Z error: can't open patch '/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/deps-download/abseil-cpp/scripts/../CMake/resolve_dependency_modules/arrow/cmake-compatibility.patch': No such file or directory
2025-10-09T03:42:02.6235097Z + exit 1
```

Pull Request resolved: facebookincubator#15113

Reviewed By: bikramSingh91

Differential Revision: D85060996

Pulled By: pedroerp

fbshipit-source-id: 255eb7a82bc3d9ffb6c47e2a4c96066b4974823e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@assignUser assignUser assignUser approved these changes

@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak is a code owner

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Zouxxyy @PHILO-HE @assignUser @facebook-github-bot