build: Add Presto native C++ dev container#25358
Conversation
|
@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. |
a1d3f8d to
235dd55
Compare
|
Hi! I've rebased and updated the devcontainer generation to be mounted from CLion, so that it uses the prestissimo dependency image leveraging us from the need to maintain the devcontainer. This should work if the dependency images work. I've also added documentation on how to configure it |
steveburnett
left a comment
There was a problem hiding this comment.
Thanks for the documentation! I made some suggestions and had questions. Let me know what you think, and ask if anything I wrote is unclear.
d1540a6 to
49afaab
Compare
49afaab to
d312725
Compare
steveburnett
left a comment
There was a problem hiding this comment.
Thanks for the doc fixes! One minor suggestion.
f9a403f to
68c61fd
Compare
czentgr
left a comment
There was a problem hiding this comment.
FYI, we also have another devcontainer setup: https://github.com/prestodb/presto-dev
But this is more about integration with CLion.
| # Copy shared libs to the directory /runtime-libraries | ||
| mkdir /runtime-libraries && \ | ||
| bash -c '!(LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/usr/local/lib:/usr/local/lib64 ldd ../cmake-build-debug/presto_cpp/main/presto_server | grep "not found")' && \ | ||
| LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/usr/local/lib:/usr/local/lib64 ldd ../cmake-build-debug/presto_cpp/main/presto_server | awk 'NF == 4 { system("cp " $3 " /runtime-libraries") }' No newline at end of file |
| @@ -0,0 +1,6 @@ | |||
| #!/bin/bash | |||
| # Before executing this script, you should compile presto_server | |||
There was a problem hiding this comment.
What happens if you don't. Will CLion throw an error?
There was a problem hiding this comment.
No, this is only to match the release environment, making sure that you're executing the server this the same libraries as the release version. This way, if your compilation lacks any library, you should see failures during the execution before the code is integrated into master
68c61fd to
48f2b73
Compare
|
Re-ran the two required and failing tests for this PR in case the failure had been a glitch of some kind. https://github.com/prestodb/presto/actions/runs/19330815080/job/57571744640?pr=25358 |
48f2b73 to
02293d2
Compare
|
HI! I've rebased my changes with master |
tdcmeehan
left a comment
There was a problem hiding this comment.
Feel free to self merge once CI is green. Thanks for the doc and this feature.
36de667 to
3eeabe5
Compare
|
@tdcmeehan tests are a bit unstable. The feature will not affect anything as it does not change any presto code. Can you merge please?
|
3eeabe5 to
10baf4c
Compare
## Description Added dev container for Presto native C++ ## Motivation and Context <!---Why is this change required? What problem does it solve?--> <!---If it fixes an open issue, please link to the issue here.--> Ease Presto native C++ development ## Impact <!---Describe any public API or user-facing feature change or any performance impact--> ## Test Plan <!---Please fill in how you tested your change--> ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == NO RELEASE NOTE == ```
Description
Added dev container for Presto native C++
Motivation and Context
Ease Presto native C++ development
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.