feat: Add tasks to manage the connector's dependencies for the worker. - #9
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request establishes the complete build infrastructure for the Velox Connector plugin. It integrates a new velox-connector Taskfile into the root Taskfile with build directory variables, defines tasks to download and install 13 C++ dependencies with coordinated CMake configuration, configures CMakeLists.txt to resolve dependency roots through Taskfile-generated settings, orchestrates generate and build tasks, and documents the build process and prerequisites. ChangesVelox Connector Build System
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@velox-connector/README.md`:
- Line 26: The README.md file is missing a trailing newline and triggers
markdownlint MD047; open the README.md (contains the line "[Task]:
https://taskfile.dev") and ensure the file ends with exactly one newline
character by adding a single newline at EOF so the file terminates with one and
only one newline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 208b6fb1-e5bd-4171-b249-2c21f62d949c
📒 Files selected for processing (7)
.gitmodulesvelox-connector/CMakeLists.txtvelox-connector/README.mdvelox-connector/taskfile.yamlvelox-connector/taskfiles/deps.yamlvelox-connector/taskfiles/velox-connector.yamlvelox-connector/tools/yscope-dev-utils
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
Nice work! Just a few small comments on a few of the dependencies we're pulling in.
On my end I think I'm probably also going to update my work for integrating libarchive into clp-s to add a flag that allows libarchive to be excluded from the build -- we shouldn't need libarchive for anything here, and including it would force us to do a lot more dependency management.
| - "filesystem" | ||
| - "iostreams" | ||
| - "process" | ||
| - "program_options" | ||
| - "regex" | ||
| - "system" | ||
| - "url" |
There was a problem hiding this comment.
I think we can get away with just regex and url actually. We don't need program_options since we aren't building executables, and I don't think clp-s uses any of the other boost libraries directly or indirectly.
There was a problem hiding this comment.
Looks like these targets are hard coded by clp's cmake.
Getting rid of any of the target results in below error when running the cmake configuration:
CMake Error at build/deps/cpp/boost-install/lib/cmake/Boost-1.87.0/BoostConfig.cmake:141 (find_package):
Could not find a configuration file for package "boost_program_options"
that exactly matches requested version "1.87.0".
The following configuration files were considered but not accepted:
/usr/local/lib/cmake/boost_program_options-1.84.0/boost_program_options-config.cmake, version: 1.84.0
Call Stack (most recent call first):
build/deps/cpp/boost-install/lib/cmake/Boost-1.87.0/BoostConfig.cmake:262 (boost_find_component)
/usr/local/share/cmake-3.28/Modules/FindBoost.cmake:594 (find_package)
build/velox-connector/_deps/clp-src/components/core/CMakeLists.txt:166 (find_package)
There was a problem hiding this comment.
I see, let's leave it for now then.
|
Addressed the comments, good catch on the dependency, lighter management is always better. Did another pass on the dependencies and seems that all are MUST have now. In the new commit, also moved yscope-dev-utils to the parent folder as this will be shared across both presto-connector and velox-connector. |
|
Actionable comments posted: 0 |
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
LGTM. For the PR title, maybe we could do something like
feat: Add taskfiles for managing Presto CLP connector dependencies.
to be a bit more specific.
20001020ycx
changed the title
|
Added
|
Also did a pass on the list of CLP-S' target that we compile for the Velox connector, it turns out that there's two more target that can be eliminated as well.
|
|
Actionable comments posted: 0 |
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
Last flag change also LGTM.
- Create root taskfile.yaml with lint, velox-connector, and utils includes - Move velox-connector/taskfiles/ to taskfiles/velox-connector/ - Rename velox-connector.yaml to main.yaml (namespace already provides prefix) - Move lint taskfile to taskfiles/lint.yaml - Move lint-requirements.txt to root - Add root .gitignore with build/ and .task/ entries - Add README.md with linting instructions - Update velox-connector/README.md build instructions - Remove build*/ from velox-connector/.gitignore (covered by root) - Remove velox-connector/taskfile.yaml and taskfiles/ - Use root-level :utils:* namespace references instead of local includes
… into feat/2026-05-22-velox-connector-taskfile
…es directory path.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
When I try to build (even before the last change I pushed), I get this error:
In file included from /home/kirk/projects/log-compressor/open-source/clp-plugin-presto-connector/velox-connector/src/protocol/../protocol/PrestoProtocolClp.h:20,
from /home/kirk/projects/log-compressor/open-source/clp-plugin-presto-connector/velox-connector/src/protocol/../protocol/ClpConnectorProtocol.h:17,
from /home/kirk/projects/log-compressor/open-source/clp-plugin-presto-connector/velox-connector/src/protocol/ClpConnectorProtocol.cpp:15:
/home/kirk/projects/log-compressor/open-source/clp-plugin-presto-connector/build/velox-connector/_deps/presto_native_execution-src/presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h:29:10: fatal error: folly/Format.h: No such file or directory
29 | #include <folly/Format.h>
| ^~~~~~~~~~~~~~~~
compilation terminated.
gmake[3]: *** [src/protocol/CMakeFiles/presto_clp_protocol.dir/build.make:76: src/protocol/CMakeFiles/presto_clp_protocol.dir/ClpConnectorProtocol.cpp.o] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:778: src/protocol/CMakeFiles/presto_clp_protocol.dir/all] Error 2
gmake[2]: *** Waiting for unfinished jobs....folly isn't listed as a requirement so we should probably be installing it via task?
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
emmmm, then I think the reason why I can compile it is because I have folly installed in the system. However, folly dependency is designed intentionally to be resolved by the presto worker at the run time as per this PR description explains. To run the presto worker, folly must be installed as a pre-requisite dependency. Therefore, for whatever missing symbols from the plugin, we will be relying on the runtime to resolve. This is to have only one version of folly when the plugin is loaded; and in fact our plugin does not use folly, it is the velox headers it includes that requires this, so we shall leave it to the presto worker to use its folly version. But I do understand this blocks compilation for the environment where it is not intended for running the presto worker, and this is indeed something we should fix. I am imagining we can create a header-only install, to provide Folly headers at compile time without statically linking Folly's library into the plugin. This shall be done not only for Folly, but some of its transitive dependencies included in Folly's header, which I propose that we shall defer this to another PR. For this PR, we will assume that system provides the Folly dependency. |
|
@kirkrodrigues Use this image to compile:
|
|
Updated the validation performed in the PR description to include the compilation tried on official Presto dev container from upstream in dockerhub: https://hub.docker.com/r/prestodb/presto-native-dependency/tags |
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Besides the inline comments, the PR description needs some work. As a reviewer, I can't follow the validation instructions and get the plugin to build (as you mentioned offline, log-surgeon fails to compile with the version of the GCC that's used by the default Presto plugin, so we should at least have mentioned that and what you did to get it to compile.). I can live with not knowing how you ran Presto to run those queries since I understand we're still in the process of putting together all the infrastructure, but at least building should be something a reviewer can validate.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat: Add tasks to manage the connector's dependencies for the worker.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
20001020ycx
changed the title
4 participants
Description
This PR adds Taskfiles for building the C++ plugin - the worker-side of the Presto CLP connector. See the updated README for usage instructions.
The goal of adding the Taskfile layer is managing the dependencies required for compiling the Velox connector. Specifically, these are the dependencies needed for compiling CLP core, since Velox's symbols are resolved at runtime (see #4 for details). The layout of the Taskfile takes log-surgeon's Taskfile infra as the reference.
Note: even though we left Velox's symbols to be resolved at runtime, the include files of Velox's dependency may still be transitively included when compiling the plugin. As of this PR, we assume the build environment is either installed with all Velox's dependency in system or a docker container with Presto dev env image. We defer these header file dependency used for compilation to another PR.
Some design notes for reviewer:
Although indeed that this PR only concerns of the CLP's dependencies, there is a subtlety worth explaining: the Presto worker and CLP may use different versions of the same library. Take fmt as an example, which did create conflict when Presto is at 0.293 and CLP is at 0.10.0, if the Presto worker uses fmt vA and CLP uses fmt vB, leaving CLP's fmt symbol unresolved at run time would lead to undefined failure. The solution is to statically link every CLP dependency into the plugin and hide all its symbols, so the plugin carries its own copies and the Presto worker never sees them. Such solution is reflected in the
deps.yamlas all dependencies were compiled with-fpicand linked statically with our plugin.Finally, I would like to make a few comments regarding what dependencies we include and exclude at
deps.yamlcompared with deps taskfile from the CLP repo.CLP_BUILD_CLP_S_*target and disablesCLP_BUILD_EXECUTABLESandCLP_BUILDING_TESTING, see here for the plugin option we set. The dependencies required for each CLP target are listed here.lz4,mongocxx,sqlite3,yaml-cppp, andzlibcatch2andliblzmautfcpp, where I don't see how CLP's CMake references it - noCLP_NEED_UTFCPPorfind_package(utfcpp)found in the CLP repo; excluded it here for lighter management.Breaking changes
None
Validation performed
Compilation Verification
Pull the dependency image
From the repository root (with submodules initialized via
git submodule update --init --recursive), start a container with the repo mounted:docker run --rm -it -v "$(pwd)":/workspace -w /workspace \ prestodb/presto-native-dependency:0.299-202606161331-03dfaf88 bashInside the container, install Task (not included in the image):
sh -c "$(curl --location https://taskfile.dev/install.sh)" -- -d -b /usr/local/binBuild, pinning the compiler to the image's GCC 11:
The plugin is produced at
build/velox-connector/libclp-plugin-velox-connector.so.Constraint Verification
We first show that this plugin shared library is self-contained, as all symbols from CLP are resolved statically. This is the constraint we have enforced when building the shared library.
nm -u libclp-plugin-velox-connector.so | grep -Ev "GLIBC|GCC|CXXABI"shows all unresolved symbols from the compiled shared library, only 104 remains unresolved:All unresolved symbols fall into two categories: (1) folly, glog, and velox symbols are the expected result because we include the Velox header, and will be provided by the Presto worker at the runtime, and (2) OpenSSL and CURL symbols are following CLP convention, as CLP assumes these are provided by the system.
Checklist
(fixes #N)or(resolves #N)syntax in the title.Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores