Skip to content

Conversation

@mblanco-denodo
Copy link
Contributor

Description

Added a dev container to develop Presto Java

Motivation and Context

Ease Presto development

Impact

Test Plan

Manual testing:

  • Created dev container in Intellij IDEA
  • Compile && mvn clean install both in java 8 and java 17 in terminal and IDEA
  • Build docs

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • 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.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

@mblanco-denodo mblanco-denodo requested a review from a team as a code owner June 17, 2025 10:59
Copy link
Contributor

@ZacBlanco ZacBlanco left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 16 to 21
texlive-fonts-recommended \
texlive-latex-recommended \
texlive-latex-extra \
latexmk \
tex-gyre \
texlive-xetex \
Copy link
Contributor

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?

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, its a large install but it is in the doc prerrequisites: https://github.com/prestodb/presto/blob/master/presto-docs/README.md

Copy link
Contributor

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

Copy link
Contributor Author

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?

@tdcmeehan tdcmeehan self-assigned this Jun 17, 2025
@mblanco-denodo
Copy link
Contributor Author

For presto native I am going to create another PR. In my experience it is better to have separate dev containers.

@steveburnett
Copy link
Contributor

@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.

@mblanco-denodo
Copy link
Contributor Author

mblanco-denodo commented Sep 9, 2025

For presto native I am going to create another PR. In my experience it is better to have separate dev containers.

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?
#25358

@tdcmeehan tdcmeehan merged commit a0c4008 into prestodb:master Sep 29, 2025
96 of 98 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