-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Add Presto Java dev container #25344
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
Conversation
ZacBlanco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I think we should consider making this container also compatible with the presto CPP development to have a unified experience.
I wonder if we could get some of the steps from presto-native-execution/scripts/setup-ubuntu to be run here as well.
Also, should we consider using something like sdkman rather than the system installs for java?
| tex-gyre \ | ||
| texlive-xetex \ | ||
| fonts-freefont-otf \ | ||
| xindy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc prerrequisites https://github.com/prestodb/presto/blob/master/presto-docs/README.md
.devcontainer/Dockerfile
Outdated
| texlive-fonts-recommended \ | ||
| texlive-latex-recommended \ | ||
| texlive-latex-extra \ | ||
| latexmk \ | ||
| tex-gyre \ | ||
| texlive-xetex \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a large install. Is installing tex actually required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, its a large install but it is in the doc prerrequisites: https://github.com/prestodb/presto/blob/master/presto-docs/README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default build does not really need the latex bits. You need them for the PDF output, but it is seldom used. I would opt to leave it out, just to keep the base image small
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I've removed that from the container, can you review again please?
|
For presto native I am going to create another PR. In my experience it is better to have separate dev containers. |
|
@mblanco-denodo, there have been some fixes to the CI tests since the last activity on this PR. If you rebase, these pending tests might pass. |
3966869 to
b091cc7
Compare
This is the one i've created some time ago for C++. Might need a refactor as the requisites for prestissimo are continuously evolving. Can you take a look into that as well? |
Description
Added a dev container to develop Presto Java
Motivation and Context
Ease Presto development
Impact
Test Plan
Manual testing:
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.