[CMake] build velox as static library #12
Conversation
fix compile/link error;
[substrait] Substrait to Velox class, change private to protected.
[Build] update thrift version
|
Important Review skippedAuto reviews are limited to specific labels. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes enhance the CMake configuration for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CMake
participant VeloxTargets
User->>CMake: Initiate Build
CMake->>VeloxTargets: Configure Installation
VeloxTargets->>CMake: Set Paths and Options
CMake->>VeloxTargets: Install Libraries
VeloxTargets->>User: Provide Installed Libraries
Poem
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
CMakeLists.txt (1)
557-561: Missing Configuration File:CMake/veloxConfig.cmake.inThe file
CMake/veloxConfig.cmake.inreferenced inCMakeLists.txtat line 558 does not exist in the repository. This will cause build or configuration errors.
CMakeLists.txtline 558:CMake/veloxConfig.cmake.inPlease ensure the file
CMake/veloxConfig.cmake.inis added to the repository or update the reference inCMakeLists.txtto point to the correct file.Analysis chain
LGTM! Verify the correctness of the new path for the configuration file.
The path for the configuration file has been updated to
CMake/veloxConfig.cmake.in. Ensure that the configuration file is correctly generated and installed in the designated location.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new path for the configuration file. # Test: Check if the configuration file is correctly generated and installed in the designated location. # Expect: The configuration file should be present at the specified path. # Run CMake configuration and check the generated configuration file. cmake . && ls "${PROJECT_BINARY_DIR}/veloxConfig.cmake"Length of output: 82
Script:
#!/bin/bash # Verify the existence of the CMake/veloxConfig.cmake.in file if [ -f "CMake/veloxConfig.cmake.in" ]; then echo "File exists: CMake/veloxConfig.cmake.in" else echo "File not found: CMake/veloxConfig.cmake.in" fi # Verify the references to CMake/veloxConfig.cmake.in in CMakeLists.txt grep -n "CMake/veloxConfig.cmake.in" CMakeLists.txtLength of output: 219
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- CMakeLists.txt (2 hunks)
- velox/common/process/CMakeLists.txt (1 hunks)
- velox/common/time/CMakeLists.txt (1 hunks)
- velox/flag_definitions/CMakeLists.txt (1 hunks)
Additional comments not posted (6)
velox/flag_definitions/CMakeLists.txt (1)
17-21: LGTM! The installation command is correctly formatted.The added
installcommand ensures that thevelox_flag_definitionslibrary is properly installed in the specified directories.velox/common/time/CMakeLists.txt (1)
21-25: LGTM! The installation command is correctly formatted.The added
installcommand ensures that thevelox_timelibrary is properly installed in the specified directories.velox/common/process/CMakeLists.txt (1)
27-31: LGTM! The installation command is correctly formatted.The added
installcommand ensures that thevelox_processtarget is properly installed in the specified directories.CMakeLists.txt (3)
551-555: LGTM! Verify the correctness of the new namespace.The namespace for the export target has been changed to
velox::. Ensure that the targets are correctly namespaced during installation.Verification successful
Namespace verification successful.
The
velox::namespace has been correctly applied in theCMakeLists.txtfile.
CMakeLists.txt:NAMESPACE velox::Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new namespace. # Test: Check if the targets are correctly namespaced during installation. # Expect: The export target should use the `velox::` namespace. # Run CMake installation and check the installed targets. cmake --install . | grep "velox::"Length of output: 107
Script:
#!/bin/bash # Description: Verify the correctness of the new namespace by searching for 'velox::' in CMake configuration files. # Search for the 'velox::' namespace in the CMake configuration files. rg 'velox::' --glob '**/CMakeLists.txt'Length of output: 74
566-571: LGTM! Verify the correctness of the file patterns for installation.The installation rules now ensure that only files matching specific patterns (like
.incand.h) are included in the installation. Ensure that the correct files are being installed.Verification successful
Verification complete: The file patterns for installation are correct for
.hfiles, but no.incfiles were found.
- The installation rules correctly include
.hfiles in theveloxdirectory.- No
.incfiles were found in theveloxdirectory, so the pattern for.incfiles might be unnecessary.Consider removing the
.incpattern if no such files are expected in the future.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the file patterns for installation. # Test: Check if the correct files are being installed. # Expect: Only files matching the specified patterns should be included in the installation. # Run CMake installation and list the installed files. cmake --install . && find "${CMAKE_INSTALL_INCLUDEDIR}/velox" -type f \( -name "*.inc" -o -name "*.h" \)Length of output: 92
Script:
#!/bin/bash # Description: Verify the correctness of the file patterns for installation. # List all .inc and .h files within the velox directory. fd -e inc -e h . veloxLength of output: 31636
31-37: LGTM! Verify the correctness of the new settings.The new variables and settings enhance the CMake configuration. Ensure that the paths and settings are correctly recognized during the build process.
Verification successful
Verification successful. The new settings in the CMakeLists.txt file are correctly defined.
The following settings were confirmed:
DEPS_INSTALLis set to "/opt/install".CMAKE_PREFIX_PATHandCMAKE_MODULE_PATHcorrectly append${DEPS_INSTALL}.Boost_USE_STATIC_RUNTIMEis set to ON.GFLAGS_USE_TARGET_NAMESPACEis set to TRUE.- Status messages for
CMAKE_PREFIX_PATHandCMAKE_MODULE_PATHare correctly configured.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new settings. # Test: Check if the new paths and settings are correctly recognized during the build process. # Expect: Status messages should display the correct paths. # Run CMake configuration and capture status messages. cmake . | grep "prefix path\|module path"Length of output: 116
Script:
#!/bin/bash # Description: Verify the correctness of the new settings in CMakeLists.txt. # Search for the DEPS_INSTALL variable setting rg 'set\(DEPS_INSTALL "/opt/install"\)' CMakeLists.txt # Search for the CMAKE_PREFIX_PATH and CMAKE_MODULE_PATH settings rg 'list\(APPEND CMAKE_PREFIX_PATH "\${DEPS_INSTALL}"\)' CMakeLists.txt rg 'list\(APPEND CMAKE_MODULE_PATH "\${DEPS_INSTALL}"\)' CMakeLists.txt # Search for the Boost_USE_STATIC_RUNTIME setting rg 'set\(Boost_USE_STATIC_RUNTIME ON\)' CMakeLists.txt # Search for the GFLAGS_USE_TARGET_NAMESPACE setting rg 'set\(GFLAGS_USE_TARGET_NAMESPACE TRUE\)' CMakeLists.txt # Search for the status messages rg 'message\(STATUS "prefix path \${CMAKE_PREFIX_PATH}"\)' CMakeLists.txt rg 'message\(STATUS "module path \${CMAKE_MODULE_PATH}"\)' CMakeLists.txtLength of output: 1253
Script:
#!/bin/bash # Description: Verify the correctness of the new settings in CMakeLists.txt. # Search for the DEPS_INSTALL variable setting rg 'set\(DEPS_INSTALL "/opt/install"\)' CMakeLists.txt # Search for the CMAKE_PREFIX_PATH and CMAKE_MODULE_PATH settings rg 'list\(APPEND CMAKE_PREFIX_PATH "\$\{DEPS_INSTALL\}"\)' CMakeLists.txt rg 'list\(APPEND CMAKE_MODULE_PATH "\$\{DEPS_INSTALL\}"\)' CMakeLists.txt # Search for the Boost_USE_STATIC_RUNTIME setting rg 'set\(Boost_USE_STATIC_RUNTIME ON\)' CMakeLists.txt # Search for the GFLAGS_USE_TARGET_NAMESPACE setting rg 'set\(GFLAGS_USE_TARGET_NAMESPACE TRUE\)' CMakeLists.txt # Search for the status messages rg 'message\(STATUS "prefix path \$\{CMAKE_PREFIX_PATH\}"\)' CMakeLists.txt rg 'message\(STATUS "module path \$\{CMAKE_MODULE_PATH\}"\)' CMakeLists.txtLength of output: 787
Summary by CodeRabbit
New Features
veloxproject and its components, improving usability and deployment across various environments.Bug Fixes