Skip to content
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

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

sitaowang1998
Copy link
Collaborator

@sitaowang1998 sitaowang1998 commented Nov 4, 2024

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

  • task:lint
  • Start devcontainer in Clion on local macOS.

Summary by CodeRabbit

  • New Features

    • Introduced a development container configuration for a consistent development environment.
    • Added scripts for automating the installation of development tools and libraries on Linux and macOS.
  • Bug Fixes

    • Updated directory paths in task configurations to ensure correct library installation locations.
  • Chores

    • Cleaned up temporary files in the Dockerfile to reduce image size.
    • Removed an outdated library installation script from the Linux directory.

Copy link

coderabbitai bot commented Nov 4, 2024

Walkthrough

This 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 dep-tasks.yaml file are updated. New scripts for installing development tools and libraries on Linux are introduced, while an existing script for macOS is modified to change its invocation path for a library installation.

Changes

File Change Summary
.devcontainer/Dockerfile Establishes a development container based on Ubuntu Jammy, sets working directory, installs development dependencies, and cleans up cached files.
.devcontainer/devcontainer.json Introduces a new configuration file specifying build settings for the development container.
cmake/Modules/FindMariaDBClientCpp.cmake Modifies CMake configuration to include the GLOBAL keyword in library imports for MariaDBClientCpp, enhancing its visibility across directories.
dep-tasks.yaml Updates directory paths for lib_install_mac and lib_install_linux tasks to point to a new subdirectory structure under lib_install.
tools/scripts/lib_install/linux/install-dev.sh Adds a new script to automate the installation of essential development tools on a Linux system.
tools/scripts/lib_install/linux/install-lib.sh Introduces a new script to automate the installation of various libraries on a Linux system.
tools/scripts/lib_install/macOS/install-lib.sh Modifies the invocation path for mariadb-connector-cpp.sh, removing the lib_install subdirectory reference.
tools/scripts/linux/install-lib.sh Deletes an existing script that automated the installation of several libraries.

Possibly related PRs

  • build: Add CMake config and libraries #13: The changes in this PR involve the addition of CMake configuration for libraries, including spdlog, fmt, and MariaDBClient, which are relevant to the installation and configuration of libraries in the main PR.
  • build: Add mariadb-connector-cpp library #18: This PR specifically adds the mariadb-connector-cpp library, which is directly related to the changes in the main PR that involve installing libraries, including the MariaDB connector.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b3c3620 and c08ef17.

📒 Files selected for processing (1)
  • tools/scripts/lib_install/macOS/install-lib.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tools/scripts/lib_install/macOS/install-lib.sh

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sitaowang1998
Copy link
Collaborator Author

Right now the container does not install python3-venv library, which means user needs to install it manually to run lint tasks. I'll add a commit to solve this later.

Copy link

@coderabbitai coderabbitai bot left a 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 requirement

The 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 robustness

Consider these improvements for better maintainability and reliability:

  1. Make GCC version configurable via build arg
  2. 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 size

The 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 and set -u are good practices, consider adding set -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 0
cmake/Modules/FindMariaDBClientCpp.cmake (1)

93-93: Excellent improvement to library visibility!

The addition of the GLOBAL keyword to the add_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

📥 Commits

Reviewing files that changed from the base of the PR and between 8491fb3 and 3c20468.

📒 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 from tools/scripts/lib_install/macOS/, the relative path .. correctly resolves to the location where mariadb-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

Comment on lines +12 to +15
"$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
Copy link

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:

  1. Hard-coded versions make updates difficult
  2. No validation of successful installations
  3. 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"

Comment on lines 1 to 7
FROM ubuntu:focal

WORKDIR /root

RUN mkdir -p ./tools/scripts/lib_install
ADD ./tools/scripts/lib_install ./tools/scripts/lib_install

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security and best practices improvements needed

Several improvements can enhance the security and maintainability of the container:

  1. Create and use a non-root user instead of running as root
  2. Replace ADD with COPY as per Docker best practices
  3. 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.

Suggested change
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)

Comment on lines 17 to 21
# 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
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Address security concerns in task runner installation

The current installation method has several security considerations:

  1. Direct script execution from the internet is risky
  2. No checksum verification
  3. 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.

Suggested change
# 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
Copy link

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.

Comment on lines 9 to 26
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
Copy link

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:

  1. Adding apt cache cleanup to reduce image size
  2. 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.

Suggested change
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/*

Comment on lines 1 to 35
#!/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
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add system validation and proper script structure.

The script should:

  1. Verify it's running on Ubuntu Focal
  2. Check for root privileges
  3. 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.

Copy link

@coderabbitai coderabbitai bot left a 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:

  1. The relationship with Ubuntu 20.04 base image
  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3c20468 and 00a746c.

📒 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, and target_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

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
tools/scripts/lib_install/linux/install-dev.sh Outdated Show resolved Hide resolved
tools/scripts/lib_install/linux/install-dev.sh Outdated Show resolved Hide resolved
tools/scripts/lib_install/macOS/install-lib.sh Outdated Show resolved Hide resolved
tools/scripts/lib_install/linux/install-dev.sh Outdated Show resolved Hide resolved
Copy link
Member

@kirkrodrigues kirkrodrigues left a 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.

@sitaowang1998 sitaowang1998 changed the title build: Add devcontainer build: Add support for a Dev Container Nov 5, 2024
@sitaowang1998 sitaowang1998 merged commit 3c89bb5 into y-scope:main Nov 5, 2024
4 checks passed
@sitaowang1998 sitaowang1998 deleted the devcontainer branch November 5, 2024 00:39
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.

2 participants