ci: extract "slim" container to run functional tests and add multiprocess and tsan, drop unneeded packages
#6604
Conversation
kwvg
force-pushed
the
ci_slim
branch
3 times, most recently
from
2667ac2 to
c48abd0
Compare
The upcoming slim container will be installing fewer packages than default to allow for TSan to work. Also lets us drop a few packages.
We don't need multiple (cross) compilers to run tests and GitHub Actions currently hamstrings our ability to thin down the runner using conventional means, so we thin down the container itself.
They don't need to be done in two separate layers, should save us a bit of time.
|
GitHub Actions run for 55a0d0d, https://github.com/kwvg/dash/actions/runs/13594731463 |
WalkthroughThis pull request implements updates across container configuration files, CI workflows, and build scripts. In the ✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ci/dash/test_integrationtests.sh (1)
67-67: Log File Handling: Switching fromcptomv
Changing from copying to moving the log files is acceptable if you intend to remove the source files after processing. Please verify that no subsequent process relies on the original log files remaining in the source directory.ci/dash/build_src.sh (1)
69-73: Conditional Linting Execution
Delegating linting to${BASE_ROOT_DIR}/ci/dash/lint-tidy.shwhenRUN_TIDYis true and not running in GitHub Actions is a cleaner approach. Please ensure that the linting script correctly propagates any errors so that lint failures aren’t silently ignored.contrib/containers/README.md (1)
3-4: Documentation Grammar Correction
The sentence “Containers that depends on other containers require BuildKit…” should be updated to “Containers that depend on other containers require BuildKit…” for correct subject-verb agreement.🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: The plural noun “Containers” expects the verb form “depend”.
Context: ...n files for containers. Containers that depends on other containers require BuildKit to...(NNS_THAT_VBZ)
contrib/containers/ci/ci.Dockerfile (1)
36-39: Update Alternatives Command Chain
The block that sets alternatives for MinGW compilers includes anexit 0at the end. This command is unnecessary and might mask potential errors in the preceding commands. Consider removingexit 0to ensure proper error propagation during the build.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.dockerignore(1 hunks).github/workflows/build-container.yml(2 hunks).github/workflows/build-depends.yml(1 hunks).github/workflows/build-src.yml(2 hunks).github/workflows/build.yml(2 hunks).gitlab-ci.yml(3 hunks)ci/dash/build_src.sh(3 hunks)ci/dash/lint-tidy.sh(1 hunks)ci/dash/slim-workspace.sh(1 hunks)ci/dash/test_integrationtests.sh(1 hunks)contrib/containers/README.md(1 hunks)contrib/containers/ci/Dockerfile(0 hunks)contrib/containers/ci/ci-slim.Dockerfile(1 hunks)contrib/containers/ci/ci.Dockerfile(1 hunks)contrib/containers/develop/Dockerfile(2 hunks)
💤 Files with no reviewable changes (1)
- contrib/containers/ci/Dockerfile
✅ Files skipped from review due to trivial changes (1)
- ci/dash/slim-workspace.sh
🧰 Additional context used
🪛 LanguageTool
contrib/containers/README.md
[grammar] ~3-~3: The plural noun “Containers” expects the verb form “depend”.
Context: ...n files for containers. Containers that depends on other containers require BuildKit to...
(NNS_THAT_VBZ)
🔇 Additional comments (28)
.dockerignore (1)
2-3: Dockerfile Inclusion Update
The explicit inclusion ofci.Dockerfileandci-slim.Dockerfilealigns with the updated container build processes. Ensure that these are the only Dockerfiles meant to be included in the build context.ci/dash/build_src.sh (1)
14-14: Environment Cleanup Simplification
Combining the unsetting ofCC,CXX, andDISPLAYinto a single line improves conciseness and clarity.contrib/containers/README.md (1)
6-12: Container Table Clarity
The table clearly outlines the dependencies and purposes of the containers, which aids in understanding the build configuration.contrib/containers/ci/ci.Dockerfile (5)
1-4: Dockerfile Inheritance and Syntax Directive
Starting with the syntax directive and inheriting from./ci-slim.Dockerfileis correctly done. Please verify that the relative path is accurate within the build context.
41-55: Clang/LLVM Installation and Alternatives Setup
The installation commands for Clang, LLVM, and related tools are comprehensive. Just ensure that the package names and the variable${LLVM_VERSION}reflect the intended versions available in the base image.
65-67: LD_LIBRARY_PATH and IWYU Build Process
SettingLD_LIBRARY_PATHand building “include-what-you-use” is well organized. Verify that removing the cloned repository post-installation does not impact any debugging needs.
75-81: Cache Directory and Ownership Setup
The creation of cache directories and adjusting ownership using${USER_ID}and${GROUP_ID}is implemented correctly. Double-check that these variables are set appropriately to avoid permission issues.
82-84: Reverting to Non-Privileged User
Switching back to the non-privileged userdashis a sound security practice once all required installations are completed.ci/dash/lint-tidy.sh (5)
1-9: Initialization and Environment Setup Are ClearThe shebang, copyright notice, locale setting, and strict bash options are set up correctly. These early lines ensure portability and robust error handling.
10-13: Clear Warning and Prerequisite GuidanceThe warning comments (lines 10–12) clearly notify users about the need to generate the compilation database and set the correct build parameters before executing the script.
14-15: Directory Navigation and Linting ExecutionThe change directory command using
${BASE_ROOT_DIR}and${BUILD_TARGET}(line 14) and the subsequent execution ofrun-clang-tidypiped throughgrep(line 15) are implemented correctly.
Suggestion: Ensure that environment variables likeBASE_ROOT_DIRandMAKEJOBSare reliably defined in all runtime contexts.
17-25: IWYU Analysis IntegrationThe use of
iwyu_tool.pywith multiple paths and the mapping file (lines 18–25) is well structured. Redirecting the output to a temporary file usingteeis appropriate for subsequent processing.
27-29: Automatic Include Fixing and Diff ReportingCalling
fix_includes.pywith the IWYU output and displaying the differences viagit diff(lines 27–29) provides a streamlined post-processing step..github/workflows/build-src.yml (2)
73-76: Updated CCache Key with New Dockerfile ReferenceThe cache key now references
contrib/containers/ci/ci.Dockerfilealong with other dynamic inputs (lines 73–76), ensuring proper cache invalidation when the Dockerfile or dependent packages change.
90-97: Addition of Linting Step for Multiprocess BuildsThe new “Run linters” step (lines 90–97) for builds with the
linux64_multiprocesstarget is integrated correctly. It correctly exportsBUILD_TARGET, sources the matrix script, and calls the newlint-tidy.shscript..github/workflows/build.yml (3)
173-178: Transition to Slim Container for TestingUpdating the
test-linux64job (lines 173–178) to depend oncontainer-slimand using its output forcontainer-pathis a well-aligned change to leverage the slimmer build environment.
Follow-up: Verify that the slim container includes all the necessary binaries and dependencies for running these tests.
179-187: Addition of Multiprocess Test JobThe new
test-linux64_multiprocessjob (lines 179–187) follows a similar pattern by sourcing the container path fromcontainer-slim. This change is consistent and supports the refactored CI strategy for multiprocess builds.
207-214: TSAN Test Job Using Slim ContainerThe configuration for the
test-linux64_tsanjob (lines 207–214) correctly usescontainer-slim. This ensures that TSAN-specific tests run in the intended environment..gitlab-ci.yml (2)
32-33: Enablement of Docker BuildKitThe export of the environment variable
DOCKER_BUILDKIT=1(line 32) is an important addition that enables BuildKit for improved build performance and capabilities on GitLab CI.
60-61: Correct Dockerfile Reference for Cache KeyUpdating the cache key to reference
contrib/containers/ci/ci.Dockerfile(lines 60–61) assures that caching is based on the correct, updated Dockerfile..github/workflows/build-container.yml (2)
5-17: Introduction of Dynamic Inputs for Build ContextThe addition of new required input parameters (
context,file, andname) (lines 5–17) significantly enhances configurability. This allows dynamic specification of the build context and Dockerfile, which improves maintainability and reusability of the workflow.
60-70: Dynamic Docker Build and Caching ConfigurationThe modified “Build and push Docker image” step (lines 57–72) now dynamically references the inputs for context and file, sets multiple tags using both the repository and provided container name, and updates caching options. This flexibility supports a more robust CI process.
.github/workflows/build-depends.yml (1)
73-77: Updated Cache Keys AlignmentThe cache key and restore keys in the “Restore cached depends” step have been updated to reference
contrib/containers/ci/ci.Dockerfileinstead of the old path. This change correctly aligns the caching mechanism with the new Dockerfile naming convention. Please verify that any similar references in other workflows have also been updated for consistency.contrib/containers/ci/ci-slim.Dockerfile (2)
1-19: Effective Multi-Stage Build for CppcheckThe builder stage effectively compiles Cppcheck from source using a lightweight Debian-based image. The use of
set -exand subsequent cleanup (removal of temporary files and apt cache) are well implemented, ensuring a minimal build stage.
20-119: Comprehensive Setup for CI Slim ContainerThe final image stage uses
ubuntu:nobleas its base and correctly integrates the compiled Cppcheck binary, configuration files, and environment settings. The Dockerfile installs the essential packages, sets up Python via Pyenv, adds tools such as ShellCheck, and configures LLVM for sanitizer builds. Cleanup commands are in place to reduce image size.Optional improvement: Consider combining certain RUN commands to further minimize image layers, though the current implementation is already effective.
contrib/containers/develop/Dockerfile (3)
1-4: Syntax Directive and Base Image UpdateThe syntax directive is now updated to
devthefuture/dockerfile-xand the base image is set to./ci/ci.Dockerfile. This change is essential for proper Dockerfile inheritance and aligns with the objectives of the PR.
9-33: Enhanced Package Installation for Dash Qt SupportThe updated RUN command now installs additional libraries (e.g., various libxcb packages) required for supporting Dash Qt. The command sequence—including package installation with
${APT_ARGS}, followed by cleanup—is well-structured and clear.
42-47: GDB Configuration and Non-Privileged User SetupThe consolidated RUN step now handles several tasks: adding the
dockergroup, appending a sudoers entry for password-less sudo access, creating the GDB configuration directory, setting the auto-load safe path, and ensuring correct ownership of the home directory. This approach streamlines the configuration process.Suggestion: If more commands are added in the future, consider splitting them into separate RUN instructions to ease debugging and enhance readability.
Motivation
dash#6583 introduced functional tests to GitHub Actions and due to a combination of limited disk space available on runners and restrictions on ways to free up said disk space (as described in dash#6583),
multiprocessandtsanbuilds had to be excluded (commit).Despite measures to reign in disk usage (source), we are experiencing sporadic failures due to space exhaustion (build, build). To take care of this, we are going to extract portions of our
cicontainer intoci-slim, containing only the parts needed to run (sanitizer-enabled) binaries, functional tests and (some) linters.Additional Information
We have used
edrevo/dockerfile-plusto allow container definitions inherit other container definitions, which has allowed for a ready-to-use interactive container (develop) to be built on top of our CI container (ci) with ease (introduced in dash#4336).Unfortunately, since then, there has been little activity at
edrevo/dockerfile-plus, last code contribution was ~4 years ago (commit) and as this PR splits offcitoci-slim, it appearsdockerfile-plushas difficulty in working withDockerfilesthat inherit otherDockerfiles(see below).Error message:
To mitigate this, we are switching over to
devthefuture/dockerfile-x, which come with minor changes in syntax.devthefuture/dockerfile-x(likeedrevo/dockerfile-plus) relies on BuildKit, it has been explicitly enabled for GitLab, without which, it fails (build).As scoping in
Dockerfiles are relative to theDockerfileitself (source),develop/Dockerfilecan includeci/Dockerfileby setting the context one directory behind (which is possible for the rootDockerfile) but whenci/Dockerfileis parsed, it will search within thecifolder and not from the context folder set earlier and so, includingci-slim/Dockerfilewould come up empty as it would resolve toci/ci-slim/Dockerfile, which doesn't exist.To work around this, both
ciandci-slimdefinitions need to share the same directory and to allow for that,ci/Dockerfileis nowci/ci.Dockerfile, so thatdevelop/Dockerfilecan set the context a directory behind, findci/ci.Dockerfile, which will search withincifor./ci-slim.Dockerfile, which will be found successfully.We have switched away from using the official LLVM installation script to importing the repository manually as we need only a select few packages to run sanitizer-enabled builds and functional tests and the script only gives you a choice between default packages and all packages.
ci-slimcontainer was most desirable, doing so breaks sanitizer builds (build)Space occupied by containers.
Calculated by modifying
develop/docker-compose.ymlto point at differentDockerfiles and building them withdocker compose, then passing them to dive v0.12.0.cidevelop(26ea618)developdevelop(26ea618)ci-slimcidevelopChecklist