-
Notifications
You must be signed in to change notification settings - Fork 3
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
build: Add support for a Dev Container #19
Conversation
WalkthroughThis pull request introduces several changes related to the development environment setup and library management. A new Dockerfile and configuration file for a development container are added, facilitating a consistent environment. Additionally, modifications to CMake scripts enhance the visibility of the MariaDBClientCpp library. The paths for library installation tasks in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Right now the container does not install |
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.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
tools/scripts/lib_install/macOS/install-lib.sh (1)
17-17
: Consider documenting the MariaDB Connector version requirementThe script pins MariaDB Connector C++ to version 1.1.5. Consider adding a comment explaining why this specific version is required, or if it should be updated.
+"# MariaDB Connector C++ 1.1.5 is required for compatibility with our current database schema" "${lib_install_scripts_dir}"/mariadb-connector-cpp.sh 1.1.5
tools/scripts/lib_install/linux/install-lib.sh (1)
9-10
: Consider using more robust path handling.The current directory path setup could be more resilient. The parent directory calculation using
..
is fragile and might break if the script is moved or symlinked.Consider this more robust approach:
-script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" -lib_install_scripts_dir=$script_dir/.. +# Get the absolute path to the script directory +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &> /dev/null && pwd)" +# Use readlink to resolve any symlinks and get the parent directory +lib_install_scripts_dir="$(readlink -f "${script_dir}/..")".devcontainer/Dockerfile (2)
8-14
: Enhance development environment configuration robustnessConsider these improvements for better maintainability and reliability:
- Make GCC version configurable via build arg
- Add verification steps for successful installation
Suggested implementation:
+ARG GCC_VERSION=10 + -RUN ./tools/scripts/lib_install/linux/install-dev.sh +RUN ./tools/scripts/lib_install/linux/install-dev.sh \ + && gcc --version | grep -q "gcc (Ubuntu ${GCC_VERSION}" \ + || (echo "GCC ${GCC_VERSION} installation failed" && exit 1) -# Set cpp, cc, and c++ to v10 -RUN update-alternatives --set cc /usr/bin/gcc-10 -RUN update-alternatives --set c++ /usr/bin/g++-10 -RUN update-alternatives --set cpp /usr/bin/cpp-10 +# Set cpp, cc, and c++ to specified version +RUN update-alternatives --set cc /usr/bin/gcc-${GCC_VERSION} \ + && update-alternatives --set c++ /usr/bin/g++-${GCC_VERSION} \ + && update-alternatives --set cpp /usr/bin/cpp-${GCC_VERSION}
23-25
: Enhance cleanup for optimal image sizeThe current cleanup is good, but could be more thorough to minimize the image size.
Suggested enhancement:
-# Remove cached files -RUN apt-get clean \ - && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* +# Comprehensive cleanup to minimize image size +RUN apt-get clean autoclean \ + && apt-get autoremove -y \ + && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* \ + && rm -rf /var/cache/apt/archives/* \ + && rm -rf /usr/share/doc/* /usr/share/man/* /usr/share/locale/*tools/scripts/lib_install/linux/install-dev.sh (2)
1-8
: Consider adding additional shell options for robust error handling.While
set -e
andset -u
are good practices, consider addingset -o pipefail
to ensure pipeline failures are caught properly.#!/usr/bin/env bash # Exit on any error set -e # Error on undefined variable set -u + +# Exit if any command in a pipeline fails +set -o pipefail
28-35
: Add error handling for update-alternatives commands.While the alternatives setup is correct, consider adding error handling to ensure the commands succeed.
+# Function to safely set up alternatives +setup_alternative() { + if ! update-alternatives "$@"; then + echo "Failed to set up alternative: $*" >&2 + exit 1 + fi +} + # Add alternatives for cpp-10, gcc-10, and g++-10 # NOTE: We use a low priority to avoid affecting the prioritization of any existing alternatives # on the user's system. -update-alternatives --install /usr/bin/cc cc /usr/bin/gcc-10 0 \ +setup_alternative --install /usr/bin/cc cc /usr/bin/gcc-10 0 \ --slave /usr/share/man/man1/cc.1.gz cc.1.gz /usr/share/man/man1/gcc.1.gz -update-alternatives --install /usr/bin/c++ c++ /usr/bin/g++-10 0 \ +setup_alternative --install /usr/bin/c++ c++ /usr/bin/g++-10 0 \ --slave /usr/share/man/man1/c++.1.gz c++.1.gz /usr/share/man/man1/g++.1.gz -update-alternatives --install /lib/cpp cpp /usr/bin/cpp-10 0 +setup_alternative --install /lib/cpp cpp /usr/bin/cpp-10 0cmake/Modules/FindMariaDBClientCpp.cmake (1)
93-93
: Excellent improvement to library visibility!The addition of the
GLOBAL
keyword to theadd_library
commands is a beneficial change that makes the MariaDBClientCpp targets available throughout the entire CMake project. This enhancement:
- Simplifies dependency management by eliminating the need to re-import the library in subdirectories
- Reduces boilerplate code in CMakeLists.txt files
- Aligns with the devcontainer setup by ensuring consistent library access across the project
Also applies to: 97-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.devcontainer/Dockerfile
(1 hunks).devcontainer/devcontainer.json
(1 hunks)cmake/Modules/FindMariaDBClientCpp.cmake
(1 hunks)dep-tasks.yaml
(1 hunks)tools/scripts/lib_install/linux/install-dev.sh
(1 hunks)tools/scripts/lib_install/linux/install-lib.sh
(1 hunks)tools/scripts/lib_install/macOS/install-lib.sh
(1 hunks)tools/scripts/linux/install-lib.sh
(0 hunks)
💤 Files with no reviewable changes (1)
- tools/scripts/linux/install-lib.sh
✅ Files skipped from review due to trivial changes (1)
- .devcontainer/devcontainer.json
🧰 Additional context used
🪛 Hadolint
.devcontainer/Dockerfile
[error] 6-6: Use COPY instead of ADD for files and folders
(DL3020)
🔇 Additional comments (4)
tools/scripts/lib_install/macOS/install-lib.sh (1)
17-17
: Verify the new path structure for mariadb-connector-cpp.sh
The path modification assumes mariadb-connector-cpp.sh exists directly under the lib_install_scripts_dir. Let's verify this assumption.
✅ Verification successful
Path resolution for mariadb-connector-cpp.sh is correct
The script path resolution is working as intended:
- The
lib_install_scripts_dir
is correctly set to the parent directory ($script_dir/..
) - The MariaDB connector script exists at
tools/scripts/lib_install/mariadb-connector-cpp.sh
- When
install-lib.sh
runs fromtools/scripts/lib_install/macOS/
, the relative path..
correctly resolves to the location wheremariadb-connector-cpp.sh
exists
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and location of mariadb-connector-cpp.sh
# Test 1: Check if the script exists at the new location
echo "Checking for mariadb-connector-cpp.sh in the new location..."
fd --type f "mariadb-connector-cpp.sh" tools/scripts
# Test 2: Search for any other instances to ensure we're not missing any
echo "Checking for any other instances of the script..."
fd --type f --hidden --no-ignore "mariadb-connector-cpp.sh"
Length of output: 454
Script:
#!/bin/bash
# Let's verify the script's path resolution by checking the content of install-lib.sh
echo "Checking install-lib.sh content..."
cat tools/scripts/lib_install/macOS/install-lib.sh
# Also check how lib_install_scripts_dir is defined
echo -e "\nChecking for lib_install_scripts_dir definition..."
rg "lib_install_scripts_dir" tools/scripts/lib_install/macOS/install-lib.sh -A 2 -B 2
Length of output: 826
tools/scripts/lib_install/linux/install-lib.sh (1)
1-8
: LGTM! Proper shell script setup.
The script follows good practices:
- Proper shebang line
- Appropriate error handling with
set -e
- Undefined variable checking with
set -u
dep-tasks.yaml (2)
14-14
: Directory structure change looks good, eh!
The updated Mac installation path properly aligns with the new directory structure.
21-21
: Directory structure change is spot on!
The updated Linux installation path properly aligns with the new directory structure. Let's verify the existence of the installation scripts in this new location.
✅ Verification successful
Directory path change is verified and properly structured
The installation scripts are present in the new location at tools/scripts/lib_install/linux
, confirming that both install-dev.sh
and install-lib.sh
have been properly migrated to align with the updated directory structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of installation scripts in the new directory structure
echo "Checking for installation scripts in the new Linux directory..."
fd -t f "install-.*\.sh$" "tools/scripts/lib_install/linux"
Length of output: 290
"$lib_install_scripts_dir"/fmtlib.sh 11.0.2 | ||
"$lib_install_scripts_dir"/spdlog.sh 1.14.1 | ||
"$lib_install_scripts_dir"/mariadb-connector-cpp.sh 1.1.5 | ||
"$lib_install_scripts_dir"/boost.sh 1.86.0 |
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.
🛠️ Refactor suggestion
Consider improving version management and installation validation.
The current implementation has several areas for improvement:
- Hard-coded versions make updates difficult
- No validation of successful installations
- Sequential installation might be slow for CI/CD pipelines
Consider these improvements:
-"$lib_install_scripts_dir"/fmtlib.sh 11.0.2
-"$lib_install_scripts_dir"/spdlog.sh 1.14.1
-"$lib_install_scripts_dir"/mariadb-connector-cpp.sh 1.1.5
-"$lib_install_scripts_dir"/boost.sh 1.86.0
+# Define versions in a separate config file for easier updates
+source "${lib_install_scripts_dir}/versions.conf"
+
+# Function to install and validate a library
+install_library() {
+ local script="$1"
+ local version="$2"
+ echo "Installing ${script} version ${version}..."
+ if ! "${lib_install_scripts_dir}/${script}.sh" "${version}"; then
+ echo "Failed to install ${script}" >&2
+ return 1
+ fi
+}
+
+# Parallel installation for faster builds
+install_library "fmtlib" "${FMTLIB_VERSION}" &
+install_library "spdlog" "${SPDLOG_VERSION}" &
+install_library "mariadb-connector-cpp" "${MARIADB_CPP_VERSION}" &
+install_library "boost" "${BOOST_VERSION}" &
+
+# Wait for all installations to complete
+wait
+
+# Verify all processes completed successfully
+if [ ${#PIPESTATUS[@]} -gt 0 ]; then
+ echo "One or more installations failed" >&2
+ exit 1
+fi
Create a new file versions.conf
:
# Library versions
FMTLIB_VERSION="11.0.2"
SPDLOG_VERSION="1.14.1"
MARIADB_CPP_VERSION="1.1.5"
BOOST_VERSION="1.86.0"
.devcontainer/Dockerfile
Outdated
FROM ubuntu:focal | ||
|
||
WORKDIR /root | ||
|
||
RUN mkdir -p ./tools/scripts/lib_install | ||
ADD ./tools/scripts/lib_install ./tools/scripts/lib_install | ||
|
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.
Security and best practices improvements needed
Several improvements can enhance the security and maintainability of the container:
- Create and use a non-root user instead of running as root
- Replace ADD with COPY as per Docker best practices
- Consider using a more specific base image tag for better reproducibility
Here's the suggested improvement:
FROM ubuntu:focal
-WORKDIR /root
+# Create a non-root user
+ARG USERNAME=dev
+ARG USER_UID=1000
+ARG USER_GID=$USER_UID
+RUN groupadd --gid $USER_GID $USERNAME \
+ && useradd --uid $USER_UID --gid $USER_GID -m $USERNAME
+
+WORKDIR /home/$USERNAME
RUN mkdir -p ./tools/scripts/lib_install
-ADD ./tools/scripts/lib_install ./tools/scripts/lib_install
+COPY ./tools/scripts/lib_install ./tools/scripts/lib_install
+
+# Change ownership of the installation scripts
+RUN chown -R $USERNAME:$USERNAME .
+
+USER $USERNAME
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
FROM ubuntu:focal | |
WORKDIR /root | |
RUN mkdir -p ./tools/scripts/lib_install | |
ADD ./tools/scripts/lib_install ./tools/scripts/lib_install | |
FROM ubuntu:focal | |
# Create a non-root user | |
ARG USERNAME=dev | |
ARG USER_UID=1000 | |
ARG USER_GID=$USER_UID | |
RUN groupadd --gid $USER_GID $USERNAME \ | |
&& useradd --uid $USER_UID --gid $USER_GID -m $USERNAME | |
WORKDIR /home/$USERNAME | |
RUN mkdir -p ./tools/scripts/lib_install | |
COPY ./tools/scripts/lib_install ./tools/scripts/lib_install | |
# Change ownership of the installation scripts | |
RUN chown -R $USERNAME:$USERNAME . | |
USER $USERNAME |
🧰 Tools
🪛 Hadolint
[error] 6-6: Use COPY instead of ADD for files and folders
(DL3020)
.devcontainer/Dockerfile
Outdated
# NOTE: | ||
# 1. `task` doesn't have an apt/yum package so we use its install script. | ||
# 2. We don't want to install it using `install-prebuilt-packages.sh` since users may use that on | ||
# their own machines and it would change their environment in a way that can't easily be undone. | ||
RUN sh -c "$(curl --location https://taskfile.dev/install.sh)" -- -d -b /usr/local/bin |
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.
Address security concerns in task runner installation
The current installation method has several security considerations:
- Direct script execution from the internet is risky
- No checksum verification
- No version pinning
Suggested secure implementation:
-# NOTE:
-# 1. `task` doesn't have an apt/yum package so we use its install script.
-# 2. We don't want to install it using `install-prebuilt-packages.sh` since users may use that on
-# their own machines and it would change their environment in a way that can't easily be undone.
-RUN sh -c "$(curl --location https://taskfile.dev/install.sh)" -- -d -b /usr/local/bin
+# Install task with version pinning and checksum verification
+ARG TASK_VERSION=v3.20.0
+ARG TASK_CHECKSUM=4c14532f581b8064a53a8f9c21bb930ef4400f5f98620a2be5c3a857cd5db85c
+
+RUN curl -sSL -o /tmp/task.tar.gz "https://github.com/go-task/task/releases/download/${TASK_VERSION}/task_linux_amd64.tar.gz" \
+ && echo "${TASK_CHECKSUM} /tmp/task.tar.gz" | sha256sum -c - \
+ && tar -C /usr/local/bin -xzf /tmp/task.tar.gz task \
+ && chmod +x /usr/local/bin/task \
+ && rm /tmp/task.tar.gz
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
# NOTE: | |
# 1. `task` doesn't have an apt/yum package so we use its install script. | |
# 2. We don't want to install it using `install-prebuilt-packages.sh` since users may use that on | |
# their own machines and it would change their environment in a way that can't easily be undone. | |
RUN sh -c "$(curl --location https://taskfile.dev/install.sh)" -- -d -b /usr/local/bin | |
# Install task with version pinning and checksum verification | |
ARG TASK_VERSION=v3.20.0 | |
ARG TASK_CHECKSUM=4c14532f581b8064a53a8f9c21bb930ef4400f5f98620a2be5c3a857cd5db85c | |
RUN curl -sSL -o /tmp/task.tar.gz "https://github.com/go-task/task/releases/download/${TASK_VERSION}/task_linux_amd64.tar.gz" \ | |
&& echo "${TASK_CHECKSUM} /tmp/task.tar.gz" | sha256sum -c - \ | |
&& tar -C /usr/local/bin -xzf /tmp/task.tar.gz task \ | |
&& chmod +x /usr/local/bin/task \ | |
&& rm /tmp/task.tar.gz |
RUN update-alternatives --set c++ /usr/bin/g++-10 | ||
RUN update-alternatives --set cpp /usr/bin/cpp-10 | ||
|
||
RUN ./tools/scripts/lib_install/linux/install-lib.sh |
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.
🛠️ Refactor suggestion
Improve library installation reliability and build efficiency
The current library installation lacks error handling and doesn't optimize Docker layer caching.
Suggested improvement:
-RUN ./tools/scripts/lib_install/linux/install-lib.sh
+# Copy only the library dependency files first for better layer caching
+COPY ./tools/scripts/lib_install/linux/requirements*.txt /tmp/requirements/
+
+# Install libraries and verify installation
+RUN ./tools/scripts/lib_install/linux/install-lib.sh \
+ && echo "Verifying library installations..." \
+ && ldconfig -p | grep -q "libmariadb" \
+ && echo "Library installation successful" \
+ || (echo "Library installation failed" && exit 1)
Committable suggestion skipped: line range outside the PR's diff.
apt-get update | ||
DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ | ||
ca-certificates \ | ||
checkinstall \ | ||
cmake \ | ||
curl \ | ||
g++ \ | ||
g++-10 \ | ||
gcc \ | ||
gcc-10 \ | ||
git \ | ||
libcurl4 \ | ||
libcurl4-openssl-dev \ | ||
libssl-dev \ | ||
make \ | ||
pkg-config \ | ||
python3 \ | ||
python3-pip |
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.
🛠️ Refactor suggestion
Add apt cache cleanup and organize package list.
While the package installation is comprehensive, consider:
- Adding apt cache cleanup to reduce image size
- Organizing packages by category with comments
apt-get update
DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \
+ # Build tools
ca-certificates \
checkinstall \
cmake \
- curl \
- g++ \
- g++-10 \
- gcc \
- gcc-10 \
- git \
- libcurl4 \
- libcurl4-openssl-dev \
- libssl-dev \
make \
pkg-config \
+ # Compilers
+ g++ \
+ g++-10 \
+ gcc \
+ gcc-10 \
+ # Version control
+ git \
+ # Libraries
+ curl \
+ libcurl4 \
+ libcurl4-openssl-dev \
+ libssl-dev \
+ # Python
python3 \
python3-pip
+
+# Clean up apt cache
+apt-get clean
+rm -rf /var/lib/apt/lists/*
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
apt-get update | |
DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ | |
ca-certificates \ | |
checkinstall \ | |
cmake \ | |
curl \ | |
g++ \ | |
g++-10 \ | |
gcc \ | |
gcc-10 \ | |
git \ | |
libcurl4 \ | |
libcurl4-openssl-dev \ | |
libssl-dev \ | |
make \ | |
pkg-config \ | |
python3 \ | |
python3-pip | |
apt-get update | |
DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ | |
# Build tools | |
ca-certificates \ | |
checkinstall \ | |
cmake \ | |
make \ | |
pkg-config \ | |
# Compilers | |
g++ \ | |
g++-10 \ | |
gcc \ | |
gcc-10 \ | |
# Version control | |
git \ | |
# Libraries | |
curl \ | |
libcurl4 \ | |
libcurl4-openssl-dev \ | |
libssl-dev \ | |
# Python | |
python3 \ | |
python3-pip | |
# Clean up apt cache | |
apt-get clean | |
rm -rf /var/lib/apt/lists/* |
#!/usr/bin/env bash | ||
|
||
# Exit on any error | ||
set -e | ||
|
||
# Error on undefined variable | ||
set -u | ||
|
||
apt-get update | ||
DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ | ||
ca-certificates \ | ||
checkinstall \ | ||
cmake \ | ||
curl \ | ||
g++ \ | ||
g++-10 \ | ||
gcc \ | ||
gcc-10 \ | ||
git \ | ||
libcurl4 \ | ||
libcurl4-openssl-dev \ | ||
libssl-dev \ | ||
make \ | ||
pkg-config \ | ||
python3 \ | ||
python3-pip | ||
|
||
# Add alternatives for cpp-10, gcc-10, and g++-10 | ||
# NOTE: We use a low priority to avoid affecting the prioritization of any existing alternatives | ||
# on the user's system. | ||
update-alternatives --install /usr/bin/cc cc /usr/bin/gcc-10 0 \ | ||
--slave /usr/share/man/man1/cc.1.gz cc.1.gz /usr/share/man/man1/gcc.1.gz | ||
update-alternatives --install /usr/bin/c++ c++ /usr/bin/g++-10 0 \ | ||
--slave /usr/share/man/man1/c++.1.gz c++.1.gz /usr/share/man/man1/g++.1.gz | ||
update-alternatives --install /lib/cpp cpp /usr/bin/cpp-10 0 |
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.
Add system validation and proper script structure.
The script should:
- Verify it's running on Ubuntu Focal
- Check for root privileges
- Include a trailing newline
#!/usr/bin/env bash
# Exit on any error
set -e
# Error on undefined variable
set -u
+# Verify we're on Ubuntu Focal
+if ! grep -q "Ubuntu 20.04" /etc/os-release; then
+ echo "This script requires Ubuntu 20.04 (Focal)" >&2
+ exit 1
+fi
+
+# Check for root privileges
+if [ "$(id -u)" -ne 0 ]; then
+ echo "This script must be run as root" >&2
+ exit 1
+fi
+
[rest of the script...]
+
Committable suggestion skipped: line range outside the PR's diff.
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CMakeLists.txt (2)
Line range hint
1-115
: Consider documenting devcontainer-specific build requirements.Since this file is crucial for the new devcontainer setup, consider adding comments about:
- The relationship with Ubuntu 20.04 base image
- Any devcontainer-specific build requirements or configurations
Add documentation at the top of the file:
+# This project uses CMake 3.16.3 to maintain compatibility with the devcontainer's +# Ubuntu 20.04 (Focal) base image. When using the devcontainer, all required +# dependencies are automatically installed through scripts in +# ./tools/scripts/lib_install/linux/ + # CMake 3.16.3 is the default on Ubuntu 20.04 cmake_minimum_required(VERSION 3.16.3)
Library versions in installation scripts differ from CMakeLists.txt requirements
The installation scripts in
tools/scripts/lib_install/linux/install-lib.sh
specify different versions than those required in CMakeLists.txt:
- CMake requires Boost 1.74, but script installs 1.86.0
- CMake requires fmt 8.0.1, but script installs 11.0.2
- CMake requires spdlog 1.9.2, but script installs 1.14.1
The installation scripts need to be updated to match the minimum version requirements in CMakeLists.txt, or CMakeLists.txt needs to be updated to reflect the newer versions being installed.
🔗 Analysis chain
Line range hint
1-115
: Verify library versions availability on Ubuntu 20.04.The CMakeLists.txt specifies minimum versions for several dependencies:
- Boost 1.74
- fmt 8.0.1
- spdlog 1.9.2
- MariaDBClientCpp 1.0
Please ensure these versions are available through the installation scripts in
./tools/scripts/lib_install/linux
for Ubuntu 20.04.Let's verify the installation scripts:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check library installation scripts for version specifications # Look for version specifications in installation scripts echo "Checking library versions in installation scripts:" rg -A 3 "VERSION|version" "./tools/scripts/lib_install/linux/"Length of output: 173
Script:
#!/bin/bash # Let's check if the directory structure exists and list its contents echo "Checking installation scripts directory:" ls -la ./tools/scripts/lib_install/linux/ echo -e "\nChecking for any install scripts:" find ./tools/scripts/lib_install/ -type f -name "*.sh" -exec ls -la {} \; echo -e "\nChecking content of any found install scripts:" find ./tools/scripts/lib_install/ -type f -name "*.sh" -exec cat {} \;Length of output: 9920
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.devcontainer/Dockerfile
(1 hunks)CMakeLists.txt
(1 hunks)tools/scripts/lib_install/linux/install-dev.sh
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .devcontainer/Dockerfile
- tools/scripts/lib_install/linux/install-dev.sh
🔇 Additional comments (1)
CMakeLists.txt (1)
1-2
: The CMake version change aligns with Ubuntu 20.04 but requires verification.
The change to CMake 3.16.3 is appropriate for Ubuntu 20.04 (Focal) compatibility. However, we should verify that all CMake features used in the project are supported by this version.
Let's verify CMake feature compatibility:
✅ Verification successful
CMake version 3.16.3 is compatible with the project's requirements
After reviewing the CMake files, the downgrade to version 3.16.3 is safe as the project only uses basic CMake features that have been available since much earlier versions. The codebase uses:
- Standard project configuration and language settings
- Basic variable and cache operations
- Common commands like
add_library
,target_sources
, andtarget_link_libraries
- Standard package finding functionality
No modern CMake features that were introduced after 3.16.3 are being used in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potentially incompatible CMake features
# Look for newer CMake commands or features that might not be available in 3.16.3
rg "cmake_" -A 2
rg "target_" -A 2
Length of output: 173
Script:
#!/bin/bash
# Let's try a more comprehensive search for CMake files and their contents
echo "=== Finding all CMake files ==="
fd -e cmake -e txt --type f CMakeLists
echo -e "\n=== Checking CMake commands in all CMake files ==="
for file in $(fd -e cmake -e txt --type f CMakeLists); do
echo -e "\n--- Contents of $file ---"
cat "$file"
done
echo -e "\n=== Specifically checking for modern CMake features ==="
rg -i "cmake_(dependent_option|host_system|language_compile_features|add_link_options|compile_features)"
echo -e "\n=== Checking target properties and commands ==="
rg -i "target_(compile_features|link_options|precompile_headers|sources)"
Length of output: 6608
…tall-lib.sh file Co-authored-by: kirkrodrigues <[email protected]>
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.
For the PR title, how about:
build: Add support for a Dev Container.
Description
Add basic setup for devconatiner. Devcontainer uses ubuntu focal as base, and install all the dependencies using install scripts in
./tools/scripts/lib_install/linux
.To start the devcontainer in Clion, see jetbrain document.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Chores