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.

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

[native] Add support for DEPENDENCY_DIR, INSTALL_PREFIX, PYTHON_VENV #23776

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Oct 7, 2024

Description

DEPENDENCY_DIR and INSTALL_PREFIX help developers manage dependencies.
PYTHON_VENV is used to configure the clang-format package.

Motivation and Context

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.

If release note is NOT required, use:

== NO RELEASE NOTE ==

czentgr
czentgr previously approved these changes Oct 7, 2024
Copy link
Contributor

@czentgr czentgr left a comment

Choose a reason for hiding this comment

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

Thank you!
Please update the release section in the PR description.

Copy link
Contributor

@steveburnett steveburnett 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 the doc! A couple of minor nits, and a suggestion to rephrase for easier understanding.

presto-native-execution/README.md Outdated Show resolved Hide resolved
presto-native-execution/README.md Outdated Show resolved Hide resolved
presto-native-execution/README.md Outdated Show resolved Hide resolved
presto-native-execution/README.md Outdated Show resolved Hide resolved
@majetideepak
Copy link
Collaborator Author

@steveburnett can you take another look? Thanks.

czentgr
czentgr previously approved these changes Oct 7, 2024
Copy link
Contributor

@czentgr czentgr left a comment

Choose a reason for hiding this comment

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

Looks good.

steveburnett
steveburnett previously approved these changes Oct 7, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

@majetideepak
Copy link
Collaborator Author

@tdcmeehan can you please help approve and merge this? Thanks.
This needs a committer approval due to the changes to .gitignore.

@tdcmeehan tdcmeehan merged commit f9f884d into prestodb:master Oct 8, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants