[native ] Fix setup scripts to align with Velox setup scripts by majetideepak · Pull Request #22448 · prestodb/presto

majetideepak · 2024-04-08T16:17:01Z

Description

Velox setup scripts have been modified to not install the build dependencies by default.
Fix setup scripts to adopt that behavior.
Fix setup scripts to encapsulate presto packages into a function.
Fix setup-centos8.sh script to include Velox dependencies.

Motivation and Context

Above

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

== NO RELEASE NOTE ==

czentgr · 2024-04-18T04:54:05Z

presto-native-execution/scripts/setup-macos.sh

 source "$(dirname "${BASH_SOURCE}")/../velox/scripts/setup-macos.sh"

-MACOS_DEPS="${MACOS_DEPS} bison gperf"
+MACOS_VELOX_DEPS="${MACOS_VELOX_DEPS} gperf"


Do we need to include MACOS_VELOX_DEPS?
The sourcing of the velox setup-macos script would set this but is the idea that brew will install both velox and prestissimo deps. This would then alleviate the need to run the velox script separately (prior). I found that running only the prestissimo script will not work on the Linux because dnf or apt isn't run to install all the deps (e.g. compiler and such) when run for the first time.

Perhaps we should do this similarly for Linux to make sure such deps are installed?

I fixed the macos script.
The new setup scripts by default do not install anything.
We source the Velox setup scripts and install the (Presto, Velox) build deps from package manager (optional), (Presto, Velox) deps from the package manager, and then the (Presto, Velox) packages via download.

I found that running only the prestissimo script will not work on the Linux because dnf or apt isn't run to install all the deps

Can you try now?

presto-native-execution/scripts/setup-centos.sh

majetideepak · 2024-04-19T17:16:35Z

I verified these changes locally.

majetideepak · 2024-04-19T17:16:50Z

@czentgr can you make another pass? Thanks!

kgpai

Looks good one comment.

kgpai · 2024-04-19T17:30:04Z

presto-native-execution/Makefile

 	rm -rf /tmp/docker && \
 	mkdir -p /tmp/docker && \
-	cp scripts/setup-$(CONTAINER_NAME).sh scripts/$(CONTAINER_NAME)-container.dockfile velox/scripts/setup-helper-functions.sh /tmp/docker && \
+	cp scripts/setup-$(CONTAINER_NAME).sh scripts/$(CONTAINER_NAME)-container.dockfile velox/scripts/setup-centos8.sh velox/scripts/setup-helper-functions.sh /tmp/docker && \


should this be velox/scripts/setup-${CONTAINER_NAME} ? ..

I made a comment about this but I deleted a duplikcate comment so I guess it deleted both?
Anyway I said that in prestissimo the script is called setup-centos.sh while in Velox it is setup-centos8.sh.

The CONTAINER_NAME is set to centos. So we'd need setup-${CONTAINER_NAME}8 here. But this doesn't work if the container is ubuntu. If it is only CentOS then we are ok.

I am removing this target as part of the #21929.
I am thinking we can use if-else for now :).

czentgr · 2024-04-19T23:18:02Z

presto-native-execution/scripts/setup-centos.sh

  ./configure --prefix=/usr/local/gperf/3_1 &&
  make "-j$(nproc)" &&
  make install &&
  ln -s /usr/local/gperf/3_1/bin/gperf /usr/local/bin/


Can we include a line about removing the symlink if it exists?
I've run into the problem where the symlink exists and line 45 will fail aborting the processing.

Good point. I will add a check if the target exists.

czentgr · 2024-04-19T23:21:08Z

presto-native-execution/scripts/setup-macos.sh

-MACOS_DEPS="${MACOS_DEPS} bison gperf"
 export FB_OS_VERSION=v2024.04.01.00

-export PATH=$(brew --prefix bison)/bin:$PATH


None of the prestissimo/velox deps require bison?
On Silicon Macs this will be a problem going forward but is likely set later by the users. But removing this we don't expect issues?

This is a temporary assignment on the current shell. I am thinking there should not be any issues since we don't run this script on every build and things build fine.

Also, bison is installed in the Velox setup script.

majetideepak · 2024-04-22T22:11:01Z

@czentgr, @kgpai addressed your comments.

kgpai

LGTM.

czentgr

Thank you!

Velox setup scripts have been modified to not install the build dependencies by default. Fix setup scripts to adopt that behavior. Fix setup scripts to encapsulate presto packages into a function. Fix setup-centos8.sh script to include Velox dependencies.

majetideepak force-pushed the fix-setup-script branch 2 times, most recently from d0cbecf to 012cffd Compare April 8, 2024 16:22

majetideepak changed the title ~~[native ] Fix setup script~~ [native ] Fix setup scripts Apr 10, 2024

majetideepak changed the title ~~[native ] Fix setup scripts~~ [native ] Fix setup scripts to optionally install build dependencies Apr 10, 2024

majetideepak mentioned this pull request Apr 15, 2024

[native] Prestissimo CI needs a new build image #22525

Closed

majetideepak force-pushed the fix-setup-script branch 2 times, most recently from 8bd92bc to a7b6887 Compare April 18, 2024 00:57

czentgr reviewed Apr 18, 2024

View reviewed changes

majetideepak force-pushed the fix-setup-script branch 2 times, most recently from b232b51 to 4ed3b6f Compare April 18, 2024 20:17

majetideepak marked this pull request as ready for review April 18, 2024 20:20

majetideepak requested a review from a team as a code owner April 18, 2024 20:20

majetideepak requested a review from czentgr April 18, 2024 20:27

majetideepak commented Apr 18, 2024

View reviewed changes

presto-native-execution/scripts/setup-centos.sh Outdated Show resolved Hide resolved

majetideepak force-pushed the fix-setup-script branch 3 times, most recently from 275cae1 to 322d67b Compare April 19, 2024 17:15

majetideepak requested a review from kgpai April 19, 2024 17:15

majetideepak changed the title ~~[native ] Fix setup scripts to optionally install build dependencies~~ [native ] Fix setup scripts to align with Velox setup scripts Apr 19, 2024

majetideepak force-pushed the fix-setup-script branch 2 times, most recently from 29035b5 to c43aa8c Compare April 19, 2024 17:28

kgpai reviewed Apr 19, 2024

View reviewed changes

czentgr reviewed Apr 19, 2024

View reviewed changes

majetideepak mentioned this pull request Apr 22, 2024

[native] Fix setup-centos.sh to include Velox dependencies #21976

Closed

6 tasks

majetideepak force-pushed the fix-setup-script branch from c43aa8c to 694aacb Compare April 22, 2024 22:09

kgpai approved these changes Apr 22, 2024

View reviewed changes

czentgr approved these changes Apr 22, 2024

View reviewed changes

majetideepak force-pushed the fix-setup-script branch from 694aacb to 6f93c75 Compare April 23, 2024 13:57

majetideepak merged commit e687113 into prestodb:master Apr 23, 2024

majetideepak deleted the fix-setup-script branch May 15, 2024 20:45

Conversation

majetideepak commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

majetideepak commented Apr 19, 2024

Uh oh!

majetideepak commented Apr 19, 2024

Uh oh!

kgpai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak commented Apr 22, 2024

Uh oh!

kgpai left a comment

Choose a reason for hiding this comment

Uh oh!

czentgr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

majetideepak commented Apr 8, 2024 •

edited

Loading

majetideepak Apr 18, 2024 •

edited

Loading