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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-6957][VL] Fix missing mvn when CI cache is hit #6966

Merged
merged 3 commits into from
Aug 22, 2024
Merged

[GLUTEN-6957][VL] Fix missing mvn when CI cache is hit #6966

merged 3 commits into from
Aug 22, 2024

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented Aug 21, 2024 •
edited
Loading

What changes were proposed in this pull request?

  • Fixed the missing mvn.
  • Maven setup is assigned to newly created setup_helper.sh to reduce repeat code.

@github-actions GitHub Actions
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:

@PHILO-HE
Copy link
Contributor Author

@zhztheplayer, this is simply a quick fix for missing mvn when cache is hit. The mentioned mvn wrapper is not used here.

@zhztheplayer
Copy link
Member

@zhztheplayer, this is simply a quick fix for missing mvn when cache is hit. The mentioned mvn wrapper is not used here.

This may not work in cache build?

incubator-gluten/.github/workflows/velox_backend_cache.yml

Line 84 in ed3d333

bash dev/ci-velox-buildshared-centos-8.sh

@zhztheplayer
Copy link
Member

If cache build is broken by this, could add an individual maven script file to call from both cache job and build job. @PHILO-HE

@PHILO-HE PHILO-HE force-pushed the fix-mvn branch 2 times, most recently from f8ed0e2 to f84acd5 Compare August 22, 2024 03:12
@PHILO-HE PHILO-HE changed the title [VL] Fix missing mvn when CI cache is hit [GLUTEN-6957][VL] Fix missing mvn when CI cache is hit Aug 22, 2024
@github-actions GitHub Actions
Copy link

#6957

zhztheplayer
zhztheplayer reviewed Aug 22, 2024
View reviewed changes
.github/workflows/util/setup_helper.sh
# See the License for the specific language governing permissions and
# limitations under the License.

function install_maven {
Copy link
Member

Choose a reason for hiding this comment

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

Could we add set -e? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, just updated. Thanks!

zhztheplayer
zhztheplayer previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

👍

@PHILO-HE PHILO-HE merged commit bd26d83 into apache:main Aug 22, 2024
42 of 43 checks passed
sharkdtu pushed a commit to sharkdtu/gluten that referenced this pull request Nov 11, 2024
@PHILO-HE
[GLUTEN-6957][VL] Fix missing mvn when CI cache is hit (apache#6966)
f0fd20c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rui-mo rui-mo rui-mo approved these changes

@zhztheplayer zhztheplayer zhztheplayer left review comments

Assignees
No one assigned
Labels
BUILD INFRA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PHILO-HE @zhztheplayer @rui-mo