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

chore: switch docker base image, use system rocksdb for faster compilation #487

Merged
merged 9 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/release-please.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ jobs:
- uses: actions/checkout@v4
if: ${{ needs.release-please.outputs.release_created }}

- name: Install dependencies
run: |
sudo apt-get update -y
sudo apt-get install -y librocksdb-dev

- name: Install stable toolchain
uses: actions-rs/toolchain@v1
if: ${{ needs.release-please.outputs.release_created }}
Expand Down
36 changes: 36 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -359,3 +359,39 @@ For running protolint, install it as python package `protolint-bin`:
# source /tmp/varfish-server-worker/bin/activate
# pip install protolint-bin
```

### Building from scratch
To reduce compile times, we recommend using a pre-built version of `rocksdb`, either from the system package manager or e.g. via `conda`:

```bash
# Ubuntu
sudo apt-get install librocksdb-dev

# Conda
conda install -c conda-forge rocksdb
```

In either case, either add
```toml
[env]
ROCKSDB_LIB_DIR = "/usr/lib/" # in case of the system package manager, adjust the path accordingly for conda
SNAPPY_LIB_DIR = "/usr/lib/" # same as above
```
to `.cargo/config.toml` or set the environment variables `ROCKSDB_LIB_DIR` and `SNAPPY_LIB_DIR` to the appropriate paths:

```bash
export ROCKSDB_LIB_DIR=/usr/lib/
export SNAPPY_LIB_DIR=/usr/lib/
```

By default, the environment variables are defined in the `.cargo/config.toml` as described above, i.e. may need adjustments if not using the system package manager.

To build the project, run:
Comment on lines +374 to +389
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

LGTM with a minor correction: Clear configuration instructions

The instructions for configuring the build environment using either .cargo/config.toml or environment variables are clear and provide flexibility for different setups. The note about default values in .cargo/config.toml is helpful for users to understand the existing configuration.

There's a minor typo in the comment on line 379. It should be "do" instead of "to". Please apply the following change:

-SNAPPY_LIB_DIR = "/usr/lib/"  # same as above
+SNAPPY_LIB_DIR = "/usr/lib/"  # do same as above

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~379-~379: “to” seems less likely than “do”.
Context: ..._DIR = "/usr/lib/" # same as above ``` to .cargo/config.toml or set the environ...

(AI_HYDRA_LEO_CP_TO_DO)

```bash
cargo build --release
```

To install the project locally, run:
```bash
cargo install --path .
```
30 changes: 15 additions & 15 deletions utils/docker/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,16 @@
# Builder
# ---------------------------------------------------------------------------

# Pinning Rust version for now because of this issue:
#
# - https://github.com/rust-lang/rust/issues/95926
FROM rust:1-bookworm AS builder
# Use ubuntu:noble as the base image
FROM ubuntu:noble AS builder
Comment on lines +16 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider using a stable Ubuntu LTS base image

Switching the base image to ubuntu:noble might introduce instability if noble is not a long-term support (LTS) release or is still under development. For enhanced stability and support, consider using an LTS version like ubuntu:22.04.

Apply this diff to change the base image to an LTS version:

-FROM ubuntu:noble AS builder
+FROM ubuntu:22.04 AS builder
📝 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
# Use ubuntu:noble as the base image
FROM ubuntu:noble AS builder
# Use ubuntu:22.04 as the base image
FROM ubuntu:22.04 AS builder


# Build dependencies first.
#
# Install dependencies for compilation of C code (e.g., rocksdb).
# Install Rust toolchain and dependencies for compilation of C code (e.g., rocksdb)
RUN apt-get update && \
apt-get install -y clang
# Add the needed Cargo components.
RUN rustup component add rustfmt
apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
. $HOME/.cargo/env && \
rustup component add rustfmt
Comment on lines +21 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure Rust and its environment are properly installed

Manually installing Rust using the curl script requires careful handling to set up the environment correctly. To ensure that Rust is available in subsequent commands without sourcing the environment every time, consider adding Rust to the PATH globally.

Apply this diff to update the Rust installation:

 RUN apt-get update && \
     apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
     curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
-    . $HOME/.cargo/env && \
+    echo 'source $HOME/.cargo/env' >> /root/.bashrc && \
     rustup component add rustfmt

This change adds the Rust environment to the .bashrc file, ensuring that it's loaded in all subsequent steps.

📝 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 install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
. $HOME/.cargo/env && \
rustup component add rustfmt
apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
echo 'source $HOME/.cargo/env' >> /root/.bashrc && \
rustup component add rustfmt


# Install build dependency `protoc`.
COPY utils/install-protoc.sh /tmp
RUN PREFIX=/usr/local bash /tmp/install-protoc.sh
Expand All @@ -49,18 +47,20 @@ COPY protos /usr/src/varfish-server-worker/protos/
# COPY utils/alpine-linker-script.sh /usr/src/varfish-server-worker/utils/
# RUN chmod a+rx /usr/src/varfish-server-worker/utils/alpine-linker-script.sh
# COPY .cargo /usr/src/varfish-server-worker/.cargo/
## Touch main.rs to prevent cached release build.

# Touch main.rs to prevent cached release build.
RUN touch /usr/src/varfish-server-worker/src/main.rs
# This is the actual application build.
RUN cargo build --release

# Build the application
RUN /root/.cargo/bin/cargo build --release
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify the Cargo build command

Avoid hardcoding the path to the Cargo binary. Since Rust's bin directory is added to the PATH, you can invoke cargo directly.

Apply this diff to simplify the build command:

-RUN /root/.cargo/bin/cargo build --release
+RUN cargo build --release
📝 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
RUN /root/.cargo/bin/cargo build --release
RUN cargo build --release


# ---------------------------------------------------------------------------
# Runtime
# ---------------------------------------------------------------------------

FROM debian:bookworm-slim AS runtime
FROM ubuntu:noble AS runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Align runtime base image with a stable release

Using ubuntu:noble as the runtime base image may lead to compatibility issues if noble is not an officially released or stable version. For consistency and reliability, consider using the same stable LTS version as in the builder stage.

Apply this diff to update the runtime base image:

-FROM ubuntu:noble AS runtime
+FROM ubuntu:22.04 AS runtime
📝 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:noble AS runtime
FROM ubuntu:22.04 AS runtime


# Install dependencies (and cleanup afterwards)
# Install dependencies (and cleanup afterward)
RUN apt-get update && \
apt-get install -y libsqlite3-0 && \
apt-get clean autoclean && \
Expand Down
Loading