feat(docker): auto delegate scripts to docker#41
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors developer scripts to standardize environment setup/logging and to automatically re-execute scripts inside the project’s cpp-dev:latest Docker image when invoked from the host, plus adjusts Doxygen output location.
Changes:
- Added shared
scripts/env.sh(paths + colorized logging) andscripts/docker/exec.sh(host→container delegation). - Updated core scripts (build/coverage/docs/format/lint/package) to source shared modules and use standardized logging.
- Changed Doxygen output directory to
docs/generated/and updated.gitignoreaccordingly.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/docker/exec.sh | Adds delegation logic to rerun scripts inside disposable Docker container. |
| scripts/env.sh | Centralizes project root resolution and logging helpers. |
| scripts/build.sh | Uses shared env/logging + delegates to container before building. |
| scripts/coverage.sh | Uses shared env/logging + delegates to container before coverage run. |
| scripts/docs.sh | Uses shared env/logging + delegates to container before running Doxygen. |
| scripts/format.sh | Uses shared env/logging + delegates to container before formatting. |
| scripts/lint.sh | Uses shared env/logging + delegates to container before clang-tidy run. |
| scripts/package.sh | Uses shared env/logging + delegates to container before packaging. |
| docs/Doxyfile | Moves Doxygen output under docs/generated. |
| .gitignore | Ignores generated docs under docs/generated/ (and local Claude settings). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/docker/exec.sh
Outdated
| script_relative="$(realpath --relative-to="$PROJECT_ROOT" "$caller_script")" | ||
|
|
There was a problem hiding this comment.
realpath --relative-to=... is a GNU-specific option and will fail on systems with BSD/macOS realpath (and also if realpath isn’t installed), causing delegation to break. Consider avoiding --relative-to by resolving an absolute path for caller_script and stripping the $PROJECT_ROOT/ prefix in pure bash, or add a small portable fallback (and emit a clear error via log_error when relative path resolution fails).
| script_relative="$(realpath --relative-to="$PROJECT_ROOT" "$caller_script")" | |
| local script_absolute | |
| # Prefer realpath if available, fall back to readlink -f | |
| if command -v realpath >/dev/null 2>&1; then | |
| script_absolute="$(realpath "$caller_script" 2>/dev/null)" || script_absolute="" | |
| elif command -v readlink >/dev/null 2>&1; then | |
| script_absolute="$(readlink -f "$caller_script" 2>/dev/null)" || script_absolute="" | |
| else | |
| script_absolute="" | |
| fi | |
| if [ -z "$script_absolute" ]; then | |
| log_error "docker/exec.sh: unable to resolve absolute path of caller script '${caller_script}'. Install 'realpath' or 'readlink', or run from a supported environment." | |
| return 1 | |
| fi | |
| case "$script_absolute" in | |
| "$PROJECT_ROOT"/*) | |
| script_relative="${script_absolute#"$PROJECT_ROOT"/}" | |
| ;; | |
| *) | |
| log_error "docker/exec.sh: caller script '${script_absolute}' is not under PROJECT_ROOT '${PROJECT_ROOT}'. Cannot determine relative path for container delegation." | |
| return 1 | |
| ;; | |
| esac |
scripts/env.sh
Outdated
| # | ||
| # SCRIPT_DIR must be set by the calling script before sourcing env.sh. | ||
| # ============================================================================== | ||
| PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" |
There was a problem hiding this comment.
env.sh assumes SCRIPT_DIR is set by the caller; if a script forgets to set it (or it’s empty), PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." ...)" will fail with a confusing error. Consider adding an explicit guard that checks SCRIPT_DIR is non-empty and points to an existing directory, and print a clear log_error message before returning non-zero.
| PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" | |
| if [ -z "${SCRIPT_DIR:-}" ]; then | |
| log_error "SCRIPT_DIR is not set. Please set SCRIPT_DIR before sourcing env.sh." | |
| return 1 | |
| fi | |
| if [ ! -d "$SCRIPT_DIR" ]; then | |
| log_error "SCRIPT_DIR '$SCRIPT_DIR' does not exist or is not a directory." | |
| return 1 | |
| fi | |
| if ! PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"; then | |
| log_error "Failed to determine PROJECT_ROOT from SCRIPT_DIR '$SCRIPT_DIR'." | |
| return 1 | |
| fi |
scripts/build.sh
Outdated
| BUILD_DIR="build" | ||
| INSTALL_DIR="install" | ||
|
|
||
| # Create and enter build directory | ||
| mkdir -p "$BUILD_DIR" | ||
| cd "$BUILD_DIR" | ||
|
|
||
| # Run CMake with Debug build type by default (for development) | ||
| log_step "Configuring CMake (Debug)..." | ||
| cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX="../$INSTALL_DIR" .. |
There was a problem hiding this comment.
PROJECT_ROOT is resolved in env.sh but this script still uses relative paths for BUILD_DIR and the CMake source dir (..). If the user runs ./scripts/build.sh from anywhere other than the repo root (common inside the container), it will build in the wrong directory and cmake .. may point at the wrong source tree. Consider cd "$PROJECT_ROOT" (after delegate_to_container) and/or making BUILD_DIR an absolute path under PROJECT_ROOT and invoking CMake with "$PROJECT_ROOT".
scripts/package.sh
Outdated
| BUILD_DIR="build" | ||
| INSTALL_DIR="install" | ||
|
|
||
| echo "[INFO] Building project..." | ||
|
|
||
| # Create and enter build directory | ||
| mkdir -p "$BUILD_DIR" | ||
| cd "$BUILD_DIR" | ||
|
|
||
| # Run CMake with Release build type by default (disable coverage for packaging) | ||
| log_step "Configuring CMake (Release)..." | ||
| cmake -DCMAKE_BUILD_TYPE=Release -DENABLE_COVERAGE=OFF -DCMAKE_INSTALL_PREFIX="../$INSTALL_DIR" .. | ||
|
|
There was a problem hiding this comment.
Same as build.sh: this script uses relative build/ and cmake .., which will behave differently depending on the caller’s working directory. Since PROJECT_ROOT is available, consider cd "$PROJECT_ROOT" before creating/entering the build dir (after container delegation), or use absolute BUILD_DIR + pass "$PROJECT_ROOT" to CMake for consistent behavior.
scripts/coverage.sh
Outdated
| BUILD_DIR="build" | ||
| INSTALL_DIR="install" | ||
|
|
||
| echo "Building with code coverage enabled..." | ||
|
|
||
| # Create and enter build directory | ||
| mkdir -p "$BUILD_DIR" | ||
| cd "$BUILD_DIR" | ||
|
|
||
| # Run CMake with coverage enabled | ||
| log_step "Configuring CMake with coverage enabled..." | ||
| cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_COVERAGE=ON -DCMAKE_INSTALL_PREFIX="../$INSTALL_DIR" .. | ||
|
|
There was a problem hiding this comment.
This script assumes it is run from the repo root (BUILD_DIR=build, cd build, cmake ..). If invoked from another directory (easy to do inside the dev container), it will generate coverage in the wrong location or configure the wrong source dir. Consider anchoring execution at PROJECT_ROOT (e.g., cd "$PROJECT_ROOT") and/or using absolute BUILD_DIR under PROJECT_ROOT + cmake "$PROJECT_ROOT".
scripts/lint.sh
Outdated
| BUILD_DIR="build" | ||
| TIDY_BIN=$(command -v clang-tidy || true) | ||
|
|
||
| if [ -z "$TIDY_BIN" ]; then | ||
| echo "[ERROR] clang-tidy not found. Install it first." | ||
| log_error "clang-tidy not found. Install it first." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "[INFO] Running clang-tidy over source files..." | ||
| log_info "Running clang-tidy over source files..." | ||
|
|
||
| find src/ tests/ -type f \( -name '*.cpp' -o -name '*.cxx' -o -name '*.cc' \) | while read -r file; do | ||
| echo "[TIDY] $file" | ||
| log_step "$file" | ||
| clang-tidy "$file" -p "$BUILD_DIR" || true |
There was a problem hiding this comment.
find src/ tests/ and BUILD_DIR="build" are relative to the current working directory, unlike format.sh which uses "$PROJECT_ROOT"/.... If lint.sh is run from outside the repo root (including inside the container), it may scan the wrong paths and point clang-tidy at the wrong build directory. Consider using ${PROJECT_ROOT}/src, ${PROJECT_ROOT}/tests, and ${PROJECT_ROOT}/build consistently.
scripts/docs.sh
Outdated
| log_info "Doxygen version: $(doxygen --version)" | ||
|
|
||
| # Run doxygen from project root | ||
| doxygen "$DOCS_DIR/Doxyfile" |
There was a problem hiding this comment.
This script now sets OUTPUT_DIRECTORY = docs/generated, which Doxygen documents as being relative to the directory where Doxygen is started. Since docs.sh doesn’t cd "$PROJECT_ROOT" before running doxygen, running the script from another directory can generate output in an unexpected location. Consider changing into PROJECT_ROOT (after container delegation) before invoking Doxygen so output is deterministic.
| doxygen "$DOCS_DIR/Doxyfile" | |
| ( | |
| cd "$PROJECT_ROOT" | |
| doxygen "docs/Doxyfile" | |
| ) |
Introduce scripts/env.sh with colored logging helpers and project path resolution, and scripts/docker/exec.sh with a delegate_to_container function that transparently re-executes scripts inside a disposable Docker container (docker run --rm) when invoked from the host.
Each script now sources env.sh for colored logging and docker/exec.sh for automatic container delegation. Plain echo statements replaced with log_info, log_step, log_warn, and log_error helpers.
Doxygen was outputting html/ and latex/ at the project root because OUTPUT_DIRECTORY was empty. Set it to docs/generated/ to keep generated docs contained. Update .gitignore accordingly.
wambitz
force-pushed
the
feature/39/auto-delegate-scripts-to-docker
branch
from
d5d1810 to
02a4a75
Compare
- Add SCRIPT_DIR guard in env.sh to catch missing/invalid values - Replace GNU-specific realpath --relative-to with portable bash string substitution in docker/exec.sh - Add cd "$PROJECT_ROOT" to all scripts so they work from any working directory - Use absolute paths for CMAKE_INSTALL_PREFIX instead of relative ../
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/env.sh
Outdated
| log_info() { echo -e "${GREEN}[INFO]${RESET} $*"; } | ||
| log_warn() { echo -e "${YELLOW}[WARN]${RESET} $*"; } | ||
| log_error() { echo -e "${RED}[ERROR]${RESET} $*"; } | ||
| log_step() { echo -e "${CYAN}[STEP]${RESET} $*"; } | ||
| log_docker() { echo -e "${BLUE}[CONTAINER]${RESET} $*"; } |
There was a problem hiding this comment.
The logging helpers use echo -e and $*, which can produce inconsistent behavior (escape processing varies) and can mangle messages that contain backslashes or leading flags. Consider switching these helpers to printf and using "$@"/"$*" safely, and also route log_warn/log_error output to stderr so pipelines can distinguish normal output from warnings/errors.
| log_info() { echo -e "${GREEN}[INFO]${RESET} $*"; } | |
| log_warn() { echo -e "${YELLOW}[WARN]${RESET} $*"; } | |
| log_error() { echo -e "${RED}[ERROR]${RESET} $*"; } | |
| log_step() { echo -e "${CYAN}[STEP]${RESET} $*"; } | |
| log_docker() { echo -e "${BLUE}[CONTAINER]${RESET} $*"; } | |
| # Logging helpers | |
| # ============================================================================== | |
| log_info() { | |
| printf '%b' "${GREEN}[INFO]${RESET}" | |
| printf ' %s' "$@" | |
| printf '\n' | |
| } | |
| log_warn() { | |
| { | |
| printf '%b' "${YELLOW}[WARN]${RESET}" | |
| printf ' %s' "$@" | |
| printf '\n' | |
| } >&2 | |
| } | |
| log_error() { | |
| { | |
| printf '%b' "${RED}[ERROR]${RESET}" | |
| printf ' %s' "$@" | |
| printf '\n' | |
| } >&2 | |
| } | |
| log_step() { | |
| printf '%b' "${CYAN}[STEP]${RESET}" | |
| printf ' %s' "$@" | |
| printf '\n' | |
| } | |
| log_docker() { | |
| printf '%b' "${BLUE}[CONTAINER]${RESET}" | |
| printf ' %s' "$@" | |
| printf '\n' | |
| } |
scripts/lint.sh
Outdated
| BUILD_DIR="build" | ||
| TIDY_BIN=$(command -v clang-tidy || true) | ||
|
|
There was a problem hiding this comment.
TIDY_BIN is computed but never used, which is confusing and can drift from the actual binary invoked. Either remove TIDY_BIN entirely or use it consistently when running clang-tidy (and when checking availability).
scripts/lint.sh
Outdated
| find src/ tests/ -type f \( -name '*.cpp' -o -name '*.cxx' -o -name '*.cc' \) | while read -r file; do | ||
| echo "[TIDY] $file" | ||
| log_step "$file" | ||
| clang-tidy "$file" -p "$BUILD_DIR" || true |
There was a problem hiding this comment.
The loop invokes clang-tidy directly even though the script already resolved the tool path in TIDY_BIN. Using the resolved path avoids accidentally running a different clang-tidy from $PATH (or failing if $PATH changes).
| clang-tidy "$file" -p "$BUILD_DIR" || true | |
| "$TIDY_BIN" "$file" -p "$BUILD_DIR" || true |
scripts/format.sh
Outdated
| @@ -32,17 +37,17 @@ for ext in "${EXTENSIONS[@]}"; do | |||
| for f in $FILES; do | |||
There was a problem hiding this comment.
This runs find once per extension, which repeatedly rescans the same directories and can get slow as the repo grows. Consider a single find with grouped -name predicates (or an array of extensions compiled into one expression) and then run clang-format once per result set.
| # Build the image if it does not exist yet | ||
| if ! docker image inspect "$IMAGE_NAME" &> /dev/null; then | ||
| log_docker "Image '${IMAGE_NAME}' not found. Building it first..." | ||
| "${PROJECT_ROOT}/scripts/docker/build_image.sh" | ||
| fi |
There was a problem hiding this comment.
When Docker isn't installed or the daemon isn't reachable, the first failing docker ... invocation will surface as a low-level error. Adding an explicit preflight check (e.g., command -v docker and a lightweight docker info/docker version probe) would let you emit a clearer log_error and exit early.
Switch logging helpers from echo -e to printf for portable escape handling. Route log_warn and log_error to stderr so pipelines stay clean. Split the SCRIPT_DIR guard into separate checks with descriptive error messages and wrap PROJECT_ROOT resolution in an error-checked subshell.
Anchor BUILD_DIR and INSTALL_DIR to PROJECT_ROOT so the script works correctly when invoked from outside the repository root.
Verify Docker is installed and the daemon is reachable before attempting container delegation, providing clear error messages on failure.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/format.sh
Outdated
| else | ||
| echo "Formatting $f" | ||
| log_step "Formatting $f" | ||
| clang-format -i "$f" |
There was a problem hiding this comment.
FILES=$(find ...) followed by for f in $FILES will word-split on whitespace and can break on filenames containing spaces/tabs/newlines. Prefer iterating over find output safely (e.g., -print0 with a while IFS= read -r -d '' loop, or mapfile), instead of expanding a whitespace-separated variable.
scripts/lint.sh
Outdated
| find src/ tests/ -type f \( -name '*.cpp' -o -name '*.cxx' -o -name '*.cc' \) | while read -r file; do | ||
| echo "[TIDY] $file" | ||
| log_step "$file" | ||
| clang-tidy "$file" -p "$BUILD_DIR" || true |
There was a problem hiding this comment.
TIDY_BIN is used to check that clang-tidy exists, but the script then invokes clang-tidy directly instead of using the resolved path. This makes the preflight check ineffective if a different clang-tidy is later picked up via PATH changes/aliases; use "$TIDY_BIN" for the invocation (or drop TIDY_BIN and rely on command -v failing naturally).
| clang-tidy "$file" -p "$BUILD_DIR" || true | |
| "$TIDY_BIN" "$file" -p "$BUILD_DIR" || true |
Consolidate multiple per-extension find calls into a single invocation with grouped -name predicates. Use -print0 with read -d '' to safely handle filenames containing whitespace or special characters.
Use absolute PROJECT_ROOT-based paths for src, tests, and the build directory. Remove the TIDY_BIN preflight check since container delegation guarantees clang-tidy is always available.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
scripts/lint.sh
Outdated
| find src/ tests/ -type f \( -name '*.cpp' -o -name '*.cxx' -o -name '*.cc' \) | while read -r file; do | ||
| echo "[TIDY] $file" | ||
| log_step "$file" | ||
| clang-tidy "$file" -p "$BUILD_DIR" || true | ||
| done |
There was a problem hiding this comment.
clang-tidy failures are currently ignored (|| true), so this script will almost always exit 0 and CI / pre-push hooks won’t fail on lint issues. Consider capturing the exit status (e.g., accumulate a failure flag) and exiting non-zero when any invocation reports problems (and/or add -warnings-as-errors=* depending on the intended policy).
scripts/format.sh
Outdated
| if $CHECK_MODE; then | ||
| echo "Checking $f" | ||
| log_step "Checking $f" | ||
| clang-format --dry-run --Werror "$f" |
There was a problem hiding this comment.
The find output is stored in FILES and then iterated with for f in $FILES, which will break on filenames containing whitespace/newlines. Prefer iterating directly from find (e.g., -print0 + while IFS= read -r -d '') so all valid paths are handled correctly.
| if [ -z "${SCRIPT_DIR:-}" ]; then | ||
| log_error "SCRIPT_DIR is not set. Please set SCRIPT_DIR before sourcing env.sh." | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
env.sh uses return at top-level when SCRIPT_DIR is missing. That works when sourced, but if someone accidentally executes scripts/env.sh directly it will error with “return: can only return from a function or sourced script”. Consider adding an explicit guard that enforces sourcing (exit with a clear message) before any return statements.
Detect when env.sh is executed directly instead of sourced and exit with a clear error message, preventing confusing "return" errors.
Track lint failures and exit non-zero when clang-tidy reports issues so CI and pre-push hooks can catch them. Also switch from piped while-loop to process substitution so the failure flag propagates correctly.
The CI workflow ran cmake directly on the host, producing compile_commands.json with host paths. When lint.sh delegated to Docker, clang-tidy could not find those paths inside the container. Fix by using ./scripts/build.sh (which delegates to Docker) instead of raw cmake commands, ensuring all steps share the same filesystem context. Add scripts/test.sh for consistent test execution via Docker delegation. Remove manual apt-get installs since tools come from the container image.
CI environments (GitHub Actions) set CI=true and provide their own toolchain. Skip container delegation in that case to avoid path mismatches between host-generated build artifacts and container paths.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Document the current CI approach (apt-get + CI=true skip), explain why Docker delegation is skipped in CI (path mismatch prevention), and provide step-by-step instructions for upgrading to GHCR container images as an alternative for production projects.
wambitz
force-pushed
the
feature/39/auto-delegate-scripts-to-docker
branch
from
6083739 to
fa11cb4
Compare
Summary
This pull request refactors the project’s developer scripts to improve consistency, maintainability, and automation, especially around containerized development. The main changes introduce a shared environment setup, standardized logging, and automatic delegation of script execution to a Docker container if not already inside one. This ensures all scripts run in a consistent environment, reduces manual steps, and improves output clarity.
Containerization and Automation:
scripts/docker/exec.shmodule that detects if a script is running outside the development container and, if so, automatically re-executes it inside a disposable Docker container. This ensures all scripts run in a consistent environment and reduces manual setup for developers.build.sh,coverage.sh,docs.sh,format.sh,lint.sh,package.sh) now sourceenv.shanddocker/exec.sh, and delegate execution to the container as needed. [1] [2] [3] [4] [5] [6]Environment and Logging Standardization:
scripts/env.sh, which provides project path resolution and reusable, colorized logging functions (log_info,log_warn,log_error, etc.), replacing ad-hocechostatements across scripts.Script Structure and Consistency:
Documentation Output:
docs/generatedfor better organization of generated documentation.These changes make the developer experience smoother, reduce onboarding friction, and help ensure consistency across all development workflows.
Resolves #39