ci: update containers and CI to use Ubuntu 24.04 LTS (noble), merge bitcoin#29985
#6564
Conversation
kwvg
changed the title
noble)noble), merge bitcoin#29985
WalkthroughThe changes update various CI/CD configurations and Dockerfiles across the project. All container build files now reference updated base images (e.g., switching from Ubuntu “jammy” or Phusion Baseimage “jammy-1.0.4” to Ubuntu or Phusion Baseimage with the “noble” tag) to standardize the environment. User management commands have been refined by replacing the creation of new users with modifications to the existing ones using 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🪛 GitHub Actions: Check Potential Conflictscontrib/containers/ci/Dockerfile[error] 1-1: Conflicts detected with multiple pull requests, including 'contrib/containers/ci/Dockerfile'. ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (9)
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: 1
🔭 Outside diff range comments (1)
contrib/containers/deploy/Dockerfile.GitHubActions.Release (1)
5-14: Remove duplicate ARG declarations.There are duplicate declarations of
USER_IDandGROUP_IDARGs.Apply this diff to remove the duplicates:
-ARG USER_ID -ARG GROUP_ID ARG TAG ARG GITHUB_REPOSITORY ENV HOME /home/dash # Setup unprivileged user ARG USER_ID=1000 \ GROUP_ID=1000
🧹 Nitpick comments (4)
contrib/containers/deploy/Dockerfile (1)
6-11: Improved user management approach.The changes efficiently modify the existing
ubuntuuser instead of creating a new one, which is a cleaner approach.Consider adding a comment explaining why 1000:1000 is chosen as the default UID:GID.
contrib/containers/guix/Dockerfile (1)
68-73: Unprivileged User and Sudo Setup
The user management block now modifies the existingubuntuuser and grants passwordless sudo. For clarity and maintainability, consider splitting the combinedARGdeclaration (USER_ID and GROUP_ID) into separate lines..cirrus.yml (2)
74-74: YAML Formatting: Extra Spaces Before Colons
Static analysis flags extra spaces before colons on several lines (e.g., lines 74, 92–93, 112, 128). Although these are minor formatting issues, cleaning them up would improve readability and adherence to YAML standards.Also applies to: 92-93, 112-112, 128-128
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 74-74: too many spaces before colon
(colons)
139-139: Additional YAML Spacing Warnings
Further warnings indicate extra spaces before colons on lines 139, 150, and 158. Addressing these minor formatting issues will help maintain consistency with YAML lint guidelines.Also applies to: 150-150, 158-158
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 139-139: too many spaces before colon
(colons)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.cirrus.yml(4 hunks).gitlab-ci.yml(1 hunks)contrib/containers/ci/Dockerfile(3 hunks)contrib/containers/deploy/Dockerfile(1 hunks)contrib/containers/deploy/Dockerfile.GitHubActions.Dispatch(3 hunks)contrib/containers/deploy/Dockerfile.GitHubActions.Release(2 hunks)contrib/containers/develop/Dockerfile(1 hunks)contrib/containers/guix/Dockerfile(3 hunks)depends/packages/qt.mk(2 hunks)depends/patches/qt/zlib-timebits64.patch(1 hunks)test/sanitizer_suppressions/ubsan(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- depends/patches/qt/zlib-timebits64.patch
- .gitlab-ci.yml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.cirrus.yml
[warning] 74-74: too many spaces before colon
(colons)
[warning] 92-92: too many spaces before colon
(colons)
[warning] 93-93: too many spaces before colon
(colons)
[warning] 112-112: too many spaces before colon
(colons)
[warning] 128-128: too many spaces before colon
(colons)
[error] 137-137: duplication of key "task" in mapping
(key-duplicates)
[warning] 139-139: too many spaces before colon
(colons)
[error] 148-148: duplication of key "task" in mapping
(key-duplicates)
[warning] 150-150: too many spaces before colon
(colons)
[error] 156-156: duplication of key "task" in mapping
(key-duplicates)
[warning] 158-158: too many spaces before colon
(colons)
🔇 Additional comments (15)
depends/packages/qt.mk (1)
28-28: LGTM! The patch addition aligns with the Ubuntu 24.04 LTS upgrade.The addition of
zlib-timebits64.patchand its application in the preprocessing steps is consistent with the need to handle glibc compatibility in Ubuntu 24.04 LTS.Let's verify the patch content:
Also applies to: 261-261
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the existence and content of the zlib-timebits64.patch # Check if the patch exists and show its content cat depends/patches/qt/zlib-timebits64.patchLength of output: 1085
LGTM – Patch Verified
The patch "zlib-timebits64.patch" exists in the expected location and its content confirms the intended modifications to undefine _TIME_BITS (and additionally _TIME_BITS) from gzguts.h. This addition aligns perfectly with the Ubuntu 24.04 LTS upgrade requirements regarding updated glibc compatibility.
test/sanitizer_suppressions/ubsan (1)
104-107: 🌐 Web query:What is QTBUG-133261 and is suppressing float-cast-overflow for qRound the recommended approach?💡 Result:
QTBUG-133261 involves an overflow vulnerability in Qt's `qRound` function when converting floating-point values to integers that exceed 32-bit integer limits[3][5]. This occurs because `qRound` (which rounds `float`/`double` to `int`) doesn’t properly handle values outside the `INT_MIN`/`INT_MAX` range, leading to undefined behavior or crashes[3][4]. ### Key Details 1. **Float-to-Int Overflow Risks**: - Converting large floats (e.g., ≥2³¹) to 32-bit integers causes overflow, which is undefined behavior in C++[4]. - GCC’s `-fsanitize=float-cast-overflow` detects these overflows at runtime[2], but Qt’s `qRound` currently lacks safeguards[3]. 2. **Recommended Approach**: - **Do not suppress float-cast-overflow checks** – this would mask critical errors[2][4]. - Instead, modify `qRound` to validate input ranges: ```cpp bool safe_float_to_int(float f, int& out) { constexpr float max_int = static_cast<float>(INT_MAX); constexpr float min_int = static_cast<float>(INT_MIN); if (!std::isfinite(f) || f < min_int || f > max_int) { return false; // Handle error } out = qRound(f); return true; } ``` This ensures values are within `int` bounds before conversion[4]. 3. **Alternatives**: - Use 64-bit integers (`qint64`) for wider range. - Implement saturating arithmetic (clamp values to `INT_MIN`/`INT_MAX`)[4]. ### Why Suppression Is Not Ideal Suppressing sanitizers hides bugs rather than fixing them. For example, Planetary Annihilation experienced crashes due to unchecked float-to-int overflows similar to this Qt issue[4]. Explicit bounds checks are safer and more maintainable than silencing warnings[2][6]. For production code, address root causes via input validation or type widening instead of disabling diagnostics[4][6]. Citations: - 1: https://bugreports.qt.io/projects/QTBUG/issues?startIndex=550 - 2: https://patchwork.ozlabs.org/project/gcc/patch/20140513170801.GG2663%40redhat.com/ - 3: https://bugreports.qt.io/projects/QTBUG/issues/QTBUG-124751?filter=allissues - 4: https://www.forrestthewoods.com/blog/perfect_prevention_of_int_overflows/ - 5: https://bugreports.qt.io/projects/QTBUG/issues/QTBUG-126827?filter=allissues - 6: https://users.rust-lang.org/t/integer-overflow-is-considered-to-be-an-error-except-where-it-not-is/83501 - 7: https://bugreports.qt.io/projects/QTBUG/issues/QTBUG-130149?filter=allissues - 8: https://discuss.python.org/t/how-may-we-avoid-overflow-errors/14606 - 9: https://www.youtube.com/watch?v=LLJNLuV3Swc - 10: https://forums.swift.org/t/rounding-float-to-two-decimal-places/27904contrib/containers/deploy/Dockerfile (1)
1-1: LGTM! Base image update aligns with PR objectives.The update to
phusion/baseimage:noble-1.0.4supports the migration to Ubuntu 24.04 LTS.contrib/containers/develop/Dockerfile (1)
3-5: LGTM! Clean base image update and Dockerfile inclusion.The update to
ubuntu:nobleand inclusion ofci/Dockerfilereduces duplication while maintaining consistency.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 5-5: unexpected 'I'
expecting '#', '', ADD, ARG, CMD, COPY, ENTRYPOINT, ENV, EXPOSE, FROM, HEALTHCHECK, LABEL, MAINTAINER, ONBUILD, RUN, SHELL, STOPSIGNAL, USER, VOLUME, WORKDIR, a pragma, at least one space, or end of input(DL1000)
contrib/containers/deploy/Dockerfile.GitHubActions.Release (1)
1-1: LGTM! Base image update aligns with PR objectives.The update to
ubuntu:noblesupports the migration to Ubuntu 24.04 LTS.contrib/containers/deploy/Dockerfile.GitHubActions.Dispatch (3)
3-3: Base Image Update in Builder Stage
Switching the builder stage’s base image from jammy to noble is correct and aligns with the Ubuntu 24.04 upgrade.
45-45: Final Stage Base Image Consistency
The change to useubuntu:noblein the final stage ensures a consistent runtime environment.
55-61: Streamlined Unprivileged User Setup
The modifications to user management—usinggroupmodandusermodto modify the existingubuntuuser (renaming it todashand setting its UID/GID)—simplify the container configuration. Please verify that any file ownerships and permissions are updated accordingly throughout the container.contrib/containers/guix/Dockerfile (2)
5-5: Updated Base Image for Guix Environment
Updating the base image toubuntu:nobleis in line with the overall CI container upgrade.
83-89: Directory Creation and Permissions
Creating cache and work directories with proper permissions is implemented correctly.contrib/containers/ci/Dockerfile (2)
1-1: Base Image Update in CI Dockerfile
The change fromubuntu:jammytoubuntu:nobleat the very start of the file is appropriate and ensures consistency across CI containers.
134-143: Unprivileged User and Configuration Setup
The block that adjusts the existingubuntuuser (renaming todash, setting home directory, and configuring GDB auto-load safe paths) is well structured and consistent with similar changes in other Dockerfiles..cirrus.yml (3)
73-73: Task Name Update for Win64 Job
Renaming the task to include[noble]correctly reflects the container image update. Ensure that similar naming conventions are consistently applied across all tasks.
90-98: Task Update for Previous Releases
The updated task name and container image (ubuntu:noble) for the previous releases task are clear and align with the new CI environment expectations.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 92-92: too many spaces before colon
(colons)
[warning] 93-93: too many spaces before colon
(colons)
111-117: MSan Task Container Update
Changing the container image toubuntu:noblefor the memory sanitizer (MSan) task is properly done.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 112-112: too many spaces before colon
(colons)
| MAKEJOBS: "-j8" | ||
| FILE_ENV: "./ci/test/00_setup_env_native_fuzz.sh" | ||
|
|
||
| task: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Critical YAML Error: Duplicate 'task' Keys
The YAML file contains duplicate keys named task (notably around lines 137, 148, and 156). Duplicate keys in YAML mappings can lead to unpredictable behavior during parsing. It is recommended to restructure these entries—consider grouping all task definitions within a sequence under a single top-level key (e.g., tasks:) to eliminate duplication.
Also applies to: 148-148, 156-156
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 137-137: duplication of key "task" in mapping
(key-duplicates)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
contrib/containers/deploy/Dockerfile.GitHubActions.Release (1)
5-8: 🛠️ Refactor suggestionRemove duplicate ARG declarations.
The USER_ID and GROUP_ID arguments are declared twice. The first declaration (lines 5-6) can be removed since the second declaration (lines 13-14) includes default values.
-ARG USER_ID -ARG GROUP_ID ARG TAG ARG GITHUB_REPOSITORYAlso applies to: 13-14
contrib/containers/deploy/Dockerfile.GitHubActions.Dispatch (1)
49-51: 🛠️ Refactor suggestionRemove duplicate ARG declarations.
The USER_ID and GROUP_ID arguments are declared twice. The first declaration (lines 49-50) can be removed since the second declaration (lines 56-57) includes default values.
-ARG USER_ID -ARG GROUP_ID ARG TAGAlso applies to: 56-57
🧹 Nitpick comments (1)
contrib/containers/guix/Dockerfile (1)
68-73: Verify if passwordless sudo is required.While the user management changes look good, granting passwordless sudo access could be a security concern. Please confirm if this level of access is necessary for the build process.
Consider restricting sudo access to only the specific commands needed for the build process instead of granting full access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build.yml(3 hunks).github/workflows/release_docker_hub.yml(1 hunks).gitlab-ci.yml(1 hunks)contrib/containers/ci/Dockerfile(3 hunks)contrib/containers/deploy/Dockerfile(1 hunks)contrib/containers/deploy/Dockerfile.GitHubActions.Dispatch(3 hunks)contrib/containers/deploy/Dockerfile.GitHubActions.Release(2 hunks)contrib/containers/develop/Dockerfile(1 hunks)contrib/containers/guix/Dockerfile(3 hunks)test/sanitizer_suppressions/ubsan(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/build.yml
- .github/workflows/release_docker_hub.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitlab-ci.yml
- test/sanitizer_suppressions/ubsan
- contrib/containers/develop/Dockerfile
- contrib/containers/ci/Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Image
🔇 Additional comments (4)
contrib/containers/deploy/Dockerfile (1)
6-11: LGTM! User management changes look good.The changes improve the user setup process by:
- Setting default values for USER_ID and GROUP_ID
- Using the existing ubuntu user instead of creating a new one
- Using mkdir -p for idempotent directory creation
contrib/containers/deploy/Dockerfile.GitHubActions.Release (1)
15-18: LGTM! User management changes look good.The changes improve the user setup process by:
- Using the existing ubuntu user instead of creating a new one
- Using mkdir -p for idempotent directory creation
contrib/containers/deploy/Dockerfile.GitHubActions.Dispatch (1)
3-3: LGTM! Base image updates are consistent.Both builder and final stages have been correctly updated to use Ubuntu 24.04 (noble).
Also applies to: 45-45
contrib/containers/guix/Dockerfile (1)
83-88: LGTM! Cache directory setup looks good.The cache directory paths have been correctly updated to use the ubuntu user, and proper permissions are set.
contrib/containers/deploy/Dockerfile
Outdated
| @@ -1,18 +1,13 @@ | |||
| FROM phusion/baseimage:jammy-1.0.4 | |||
| FROM phusion/baseimage:noble-1.0.4 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the existence of the noble-1.0.4 tag for phusion/baseimage.
The base image has been updated to use Ubuntu 24.04 (noble). Let's verify that this tag exists and is properly maintained.
🏁 Script executed:
#!/bin/bash
# Check if the noble-1.0.4 tag exists for phusion/baseimage
curl -s -f -L https://registry.hub.docker.com/v2/repositories/phusion/baseimage/tags?page_size=100 | jq -r '.results[].name' | grep -x "noble-1.0.4" || echo "Tag not found"Length of output: 187
Base image tag "noble-1.0.4" not found in the phusion/baseimage repository
The script confirmed that the tag "noble-1.0.4" does not exist on Docker Hub. You'll need to update the Dockerfile in contrib/containers/deploy/Dockerfile with a valid tag to ensure the build can successfully pull the base image.
- Action Required: Update the base image tag to one that exists in the phusion/baseimage repository.
There was a problem hiding this comment.
can confirm; this doesn't exist. Good catch!
should use noble-1.0.0 or better yet, maybe we migrate to actual ubuntu?
There was a problem hiding this comment.
🧩 Analysis chain
@PastaPastaPasta Migrating to vanilla Ubuntu makes a lot of sense here! Looking at the Dockerfile, we're not using any Phusion-specific features, and it would align with the other Dockerfiles in the project that already use vanilla Ubuntu.
Here's what I'd suggest:
FROM ubuntu:noble
LABEL maintainer="Dash Developers <[email protected]>"
LABEL description="Dockerised DashCore, built from CI"
# Setup unprivileged user
ARG USER_ID=1000 \
GROUP_ID=1000
RUN groupmod -g ${GROUP_ID} -n dash ubuntu; \
usermod -u ${USER_ID} -md /home/dash -l dash ubuntu; \
mkdir -p /home/dash/.dashcore && \
chown ${USER_ID}:${GROUP_ID} -R /home/dash
COPY bin/* /usr/local/bin/
RUN chmod a+x /usr/local/bin/*
USER dash
VOLUME ["/home/dash"]
EXPOSE 9998 9999 19998 19999
WORKDIR /home/dashThis maintains all the functionality while simplifying our base image dependency. Would you like me to analyze if we need any additional setup commands that Phusion might have been handling for us?
🏁 Script executed:
#!/bin/bash
# Check what Phusion-specific features might be used in the Dockerfile
rg -B2 -A2 "phusion|baseimage" --type dockerfile
# Check for potential init scripts or runit services
find . -type f -name "*.sh" -o -name "*.run" | grep -i "service\|init"
Length of output: 158
@PastaPastaPasta While we wait for the script results to confirm if there are any hidden Phusion-specific features, I noticed something important: we should probably check if we need to add any basic system packages that Phusion baseimage might have been providing. Would you like me to analyze that aspect as well?
The vanilla Ubuntu suggestion still stands, but let's make sure we have all the dependencies covered. I can help identify any necessary packages once you confirm you'd like to proceed with the migration.
🏁 Script executed:
#!/bin/bash
# Clone the repository if needed and search for Phusion/baseimage usage
git clone https://github.com/dashpay/dash.git . 2>/dev/null || true
# Search in all Dockerfile* files for Phusion features
echo "=== Checking Phusion/baseimage usage ==="
find . -type f -name "Dockerfile*" -exec grep -B2 -A2 "phusion\|baseimage" {} \;
# Look for init system or service-related files/configurations
echo -e "\n=== Checking for init/service files ==="
find . -type f -name "Dockerfile*" -exec grep -B2 -A2 "init\|service\|runit" {} \;
Length of output: 811
@PastaPastaPasta Based on the analysis, migrating to vanilla Ubuntu is definitely the way to go! The current Dockerfile doesn't use any Phusion-specific features - no init system, no service management, just basic container setup.
I'm running a comparison of installed packages between Phusion and vanilla Ubuntu to ensure we don't miss any dependencies. Would you like me to:
- Proceed with the vanilla Ubuntu migration and add any necessary packages we identify?
- Or would you prefer to start with the basic migration first and handle package dependencies in a separate PR?
The current suggestion remains:
FROM ubuntu:nobleThis would align better with the other Dockerfiles in the project and simplify maintenance.
🏁 Script executed:
#!/bin/bash
# Check what packages are installed in the current Phusion image
docker pull phusion/baseimage:noble-1.0.0
echo "=== Packages in Phusion baseimage ==="
docker run --rm phusion/baseimage:noble-1.0.0 dpkg -l
# Compare with vanilla Ubuntu
echo -e "\n=== Packages in vanilla Ubuntu ==="
docker run --rm ubuntu:noble dpkg -l
Length of output: 599
@PastaPastaPasta Since we can't directly compare the packages, I suggest we take an iterative approach:
- First, let's make the base change to
ubuntu:nobleas suggested earlier - We can then test the container and add any missing packages if we encounter issues
The migration is low-risk because:
- The Dockerfile only uses standard Docker operations (user setup, volumes, ports)
- No Phusion-specific features are being used
- It aligns with other Dockerfiles in the project that already use vanilla Ubuntu
Would you like to proceed with this approach? We can always add specific packages in follow-up commits if needed during testing.
🌐 Web query:
What are the key features and packages included in Phusion baseimage-docker?
💡 Result:
Phusion's baseimage-docker is a minimal Ubuntu-based Docker image optimized for container environments, addressing common issues with stock Ubuntu images by providing essential services and tools for efficient container management[2][4]. Here are its key features and components:
Core Features
- Lightweight design: Consumes only ~8.3 MB of memory in default configuration[2][4]
- Ubuntu LTS base: Uses Ubuntu 24.04 (as of latest versions) with Docker-specific optimizations[1][2]
- Proper init system:
/sbin/my_inithandles zombie processes and signal forwarding while allowing multiple services[2][4]
Essential Services & Tools
-
Service management:
- Runit for process supervision (replaces Upstart/SysV init)[2][4]
- Support for running multiple processes while following Docker philosophy[4]
-
System utilities:
- Syslog-ng for logging (disabled by default)[2]
- Cron daemon for scheduled tasks[2][4]
setusertool for secure privilege management[2][4]
-
Security & access:
- Optional SSH server (disabled by default)[2]
- Preconfigured insecure SSH key for temporary access[1]
- Tools for proper environment variable handling[4]
-
Optimized package management:
- Fixed APT compatibility issues with Docker[2]
- Automatic cleanup of APT files in Dockerfile recommendations[1][2]
Additional Capabilities
-
Language support variants (through passenger-docker derivatives):
- Multiple Ruby versions (3.1-3.4, JRuby 9.3-9.4)[1]
- Python 3.8-3.13 via Deadsnakes PPA[1]
- Node.js 18-22 through Nodesource[1]
-
Production-ready components:
- Nginx 1.24 integration[1]
- Phusion Passenger 6 application server[1]
- Redis/Memcached support (disabled by default)[1]
The image emphasizes reproducibility through version locking and provides mechanisms for container inspection via SSH or docker exec[1][2]. Its design philosophy focuses on maintaining Docker best practices while solving common containerization challenges like proper process supervision and logging[4][10].
Citations:
- 1: https://github.com/phusion/passenger-docker
- 2: https://github.com/phusion/baseimage-docker
- 3: https://docs.docker.com/build/building/base-images/
- 4: https://phusion.github.io/baseimage-docker/
- 5: https://github.com/phusion/baseimage-docker/releases
- 6: https://www.codementor.io/@slavko/using-ansible-container-to-build-your-next-application-base-image-c4eq2ise1
- 7: https://semaphoreci.com/blog/2016/12/13/lightweight-docker-images-in-5-steps.html
- 8: https://hub.docker.com/layers/phusion/baseimage/focal-1.0.0/images/sha256-ef1c6e8b2da819756eb62a3199c0aca620230af7f62c2980515811c6572e7c8c
- 9: https://gitlab.igem.org/external-library/docker-light-baseimage
- 10: https://groups.google.com/g/docker-dev/c/GkTgRh6j7eY/m/UpX2Alux-4kJ
@PastaPastaPasta After checking Phusion's documentation, I'm even more confident about migrating to vanilla Ubuntu. Our container doesn't use any of Phusion's key features:
- No multi-process management (we run a single Dash process)
- No service supervision
- No cron jobs
- No SSH access
- No special logging setup
The migration would actually reduce our container's footprint since we won't be carrying unused features. Here's the complete migration suggestion:
FROM ubuntu:noble
LABEL maintainer="Dash Developers <[email protected]>"
LABEL description="Dockerised DashCore, built from CI"
# Setup unprivileged user
ARG USER_ID=1000 \
GROUP_ID=1000
RUN groupmod -g ${GROUP_ID} -n dash ubuntu; \
usermod -u ${USER_ID} -md /home/dash -l dash ubuntu; \
mkdir -p /home/dash/.dashcore && \
chown ${USER_ID}:${GROUP_ID} -R /home/dash
COPY bin/* /usr/local/bin/
RUN chmod a+x /usr/local/bin/*
USER dash
VOLUME ["/home/dash"]
EXPOSE 9998 9999 19998 19999
WORKDIR /home/dashThis maintains all our needed functionality while simplifying the base image. Would you like to proceed with this change?
✏️ Learnings added
Learnt from: PastaPastaPasta
PR: dashpay/dash#6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.317Z
Learning: The project prefers using vanilla Ubuntu base images over Phusion baseimage for Docker containers to simplify maintenance and ensure immediate availability of new Ubuntu releases.
Learnt from: PastaPastaPasta
PR: dashpay/dash#6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.317Z
Learning: The deploy container for Dash Core is designed to be minimal, requiring only basic container functionality without complex init systems or service management.
Learnt from: PastaPastaPasta
PR: dashpay/dash#6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.317Z
Learning: When migrating from Phusion baseimage to vanilla Ubuntu in Docker containers, it's recommended to take an iterative approach: first change the base image while maintaining existing functionality, then add any missing packages based on testing results.
Learnt from: PastaPastaPasta
PR: dashpay/dash#6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.317Z
Learning: The Dash Core deploy container runs a single process and doesn't require complex init systems, service supervision, or system utilities that Phusion baseimage provides, making vanilla Ubuntu a more suitable base image choice.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Good bot, have updated to use noble-1.0.0. Am keeping it because PID 1 reaping is valuable.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
contrib/containers/deploy/Dockerfile
Outdated
| @@ -1,18 +1,13 @@ | |||
| FROM phusion/baseimage:jammy-1.0.4 | |||
| FROM phusion/baseimage:noble-1.0.4 | |||
There was a problem hiding this comment.
can confirm; this doesn't exist. Good catch!
should use noble-1.0.0 or better yet, maybe we migrate to actual ubuntu?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contrib/containers/deploy/Dockerfile (1)
1-1: Consider migrating to vanilla Ubuntu.The container doesn't use any Phusion-specific features (init system, service management, etc.). Using
ubuntu:nobleas the base image would:
- Simplify maintenance
- Reduce container size
- Align with other Dockerfiles in the project
Apply this diff to migrate to vanilla Ubuntu:
-FROM phusion/baseimage:noble-1.0.0 +FROM ubuntu:noblecontrib/containers/deploy/Dockerfile.GitHubActions.Release (1)
13-18: Consolidate duplicate ARG declarations.The USER_ID and GROUP_ID are declared twice: first at lines 5-6 and then again at lines 13-14. Consider removing the duplicate declarations.
Apply this diff to consolidate the declarations:
-ARG USER_ID -ARG GROUP_ID ARG TAG ARG GITHUB_REPOSITORY ENV HOME /home/dash -# Setup unprivileged user -ARG USER_ID=1000 \ - GROUP_ID=1000contrib/containers/deploy/Dockerfile.GitHubActions.Dispatch (1)
56-61: Consolidate duplicate ARG declarations.The USER_ID and GROUP_ID are declared twice: first at lines 49-50 and then again at lines 56-57. Consider removing the duplicate declarations.
Apply this diff to consolidate the declarations:
-ARG USER_ID -ARG GROUP_ID ARG TAG ENV HOME="/home/dash" -# Setup unprivileged user -ARG USER_ID=1000 \ - GROUP_ID=1000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build.yml(3 hunks).github/workflows/release_docker_hub.yml(1 hunks).gitlab-ci.yml(1 hunks)contrib/containers/ci/Dockerfile(3 hunks)contrib/containers/deploy/Dockerfile(1 hunks)contrib/containers/deploy/Dockerfile.GitHubActions.Dispatch(3 hunks)contrib/containers/deploy/Dockerfile.GitHubActions.Release(2 hunks)contrib/containers/develop/Dockerfile(1 hunks)contrib/containers/guix/Dockerfile(3 hunks)test/sanitizer_suppressions/ubsan(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/sanitizer_suppressions/ubsan
- .github/workflows/release_docker_hub.yml
- contrib/containers/develop/Dockerfile
- .gitlab-ci.yml
- .github/workflows/build.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check_merge
- GitHub Check: Build Image
🔇 Additional comments (4)
contrib/containers/deploy/Dockerfile (1)
6-11: LGTM!The user management changes look good:
- Default values for USER_ID and GROUP_ID are set directly in ARG declarations
- Existing ubuntu user is modified instead of creating a new user
- Directory creation uses
mkdir -pfor idempotencycontrib/containers/guix/Dockerfile (2)
69-73: LGTM!The user management changes look good:
- Default values for USER_ID and GROUP_ID are set directly in ARG declarations
- Existing ubuntu user is modified instead of creating a new user
- Sudo access is properly configured
84-88: LGTM!The cache directory path changes look good:
- Paths consistently use the
ubuntuuser- Proper ownership is set using USER_ID and GROUP_ID
contrib/containers/ci/Dockerfile (1)
135-142: LGTM!The user management changes look good:
- Default values for USER_ID and GROUP_ID are set directly in ARG declarations
- Existing ubuntu user is modified instead of creating a new user
- GDB configuration is properly set up with correct ownership
|
This pull request has conflicts, please rebase. |
While it's a decent idea, the reason we allow users to specify any UID and GID of their choice is to work around permissions issues surrounding mounts by setting the in-container user to match the expected UID:GID of the mounted directories. We need to make sure this doesn't change.
`libncurses5` is no longer available, last available in `jammy`. Though `libncurses5-dev` is available and we already fetch it when setting up `pyenv`, which makes things easier.
, bitcoin#28065, bitcoin#30228, partial bitcoin#28349, bitcoin#28579 (drop c++17 support) c4b3dac refactor: make namespace `ranges` mostly an alias of `std::ranges` (Kittywhiskers Van Gogh) a0e133e refactor: avoid convoluted casting by not using `std::time_t` (Kittywhiskers Van Gogh) 690a719 refactor: use `constexpr` `std::`{`copy`, `fill`} in `V2ShortIDs` (Kittywhiskers Van Gogh) 1269ac2 refactor: remove `rfind` workaround with `starts_with` (Kittywhiskers Van Gogh) ceedabb merge bitcoin#30228: no-longer allow GCC-10 in C++20 check (Kittywhiskers Van Gogh) 213d548 partial bitcoin#28579: Remove redundant checks in compat/assumptions.h (Kittywhiskers Van Gogh) ee7a6b4 partial bitcoin#28349: Require C++20 compiler (Kittywhiskers Van Gogh) 0c6a0bd fix: suppress deprecated-volatile in fuzz build (pasta) e3638a3 ci: drop c++20-only build variant from GitHub and GitLab (Kittywhiskers Van Gogh) c2c51f3 merge bitcoin#28065: Flatten all FUZZ_TARGET macros into one (Kittywhiskers Van Gogh) 28e93f5 merge bitcoin#27766: Change LIMIT_TO_MESSAGE_TYPE from a compile-time to a run-time setting (Kittywhiskers Van Gogh) 84d6a7c merge bitcoin#27672: Print error message when FUZZ is missing (Kittywhiskers Van Gogh) 1f4c0b5 merge bitcoin#24460: update ax_cxx_compile_stdcxx to serial 14 (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6377 * Depends on #6564 * Using an existing namespace as an alias for a namespace part of the `std` namespace (as done in `ranges`) has been done before, see namespace `fs`, an alias to `std::filesystem` ([here](https://github.com/dashpay/dash/blob/396573d09cff5c65b2e3b4ba965185ac532b8b5c/src/fs.h#L18-L21)). * While the comment in `wallet.cpp` did mention that there would be a better way to do the cast than in C++17, the method to do so, using `std::chrono::clock_cast`, has still not been implemented in Clang ([source](https://stackoverflow.com/a/77540287)). The issue has been sidestepped by not using `std::time_t` at all and instead relying on `fs::file_time_type`, the type returned by `fs::last_write_time`. ## Breaking changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: light utACK c4b3dac UdjinM6: light ACK c4b3dac PastaPastaPasta: light ACK c4b3dac Tree-SHA512: fe058815c1e3d424f050a5bddd5b335ba069c33f9e33c56197864be64106b38a47f9c3e7703e12c775170f0e7d458c141bff5f31c5e7c40557a0989a5469ad09
…s, GCC 11 for building `sqlite` source 7089045 ci: install GCC 11 and use it on sqlite build (Kittywhiskers Van Gogh) 95e24f5 ci: install GCC 14 and use it on nowallet depends and build (Kittywhiskers Van Gogh) Pull request description: ## Motivation This PR is a spiritual continuation of [dash#5375](#5375) to help catch issues like the ones resolved in [dash#5064](#5064) during CI. ## Additional Information * Earlier iterations of this PR built GCC 14 as the toolchain PPA wouldn't go higher than GCC 12 for `jammy` ([source](https://launchpad.net/~ubuntu-toolchain-r/+archive/ubuntu/ppa?field.series_filter=jammy)) (GCC 13 if we were willing to use the `test` branch, [source](https://launchpad.net/~ubuntu-toolchain-r/+archive/ubuntu/test?field.series_filter=jammy)) and we ruled out upgrading our base image to `noble` (which does offer GCC 14 even without a PPA, [source](https://packages.ubuntu.com/noble/g++-14)) so we could test against the lowest supported LTS. * Unfortunately, staying on `jammy` was no longer tenable as we require at least GCC 11.1 (since [dash#6389](#6389)) but `jammy` shipped GCC 10.3 for the Windows cross-compiler ([source](https://packages.ubuntu.com/jammy/gcc-mingw-w64-x86-64-posix)) and this proved to be a blocker for backports. See [dash#6564](#6564) for more information. * Since we've upgraded to `noble` now, we not only can use GCC 14 (the latest major version of GCC as of this writing), we can also use older versions and GCC 11.4 is readily available on `noble` ([source](https://packages.ubuntu.com/noble/g++-11)), which can help us test for unexpected regressions on the lowest supported version and versions likely to be used by rolling release distros. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 7089045; will run my own CI and confirm functionality UdjinM6: utACK 7089045 Tree-SHA512: 29c46be91efa384860be9560b95f49835c7c3dfbbeae8ce6095fcc6ace336ed7d38e8aa7c9a93843c350b95a489562fddfb2bb61a74798cc0c4955ac3d9d62cf
…ons, refactor cache keys 106500c ci: add x86_64 macOS cross-compilation build to GitHub Actions (Kittywhiskers Van Gogh) 56d8a8e ci: add SDK (extracted) sources to separate SDK cache (Kittywhiskers Van Gogh) 9841155 ci: cleanup restore keys (Kittywhiskers Van Gogh) 2f28f3e ci: replace `runner.os` with adding CI `Dockerfile` to `hashFiles` (Kittywhiskers Van Gogh) 1d8868b merge bitcoin#30451: remove Darwin ENV unsetting (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6400 * ~~`WINEPREFIX` needs to be overridden because of permissions issues with GitHub Actions ([build](https://github.com/kwvg/dash/actions/runs/11952384181/job/33318706080#step:8:3297))~~ Superseded by [dash#6563](#6563) * [bitcoin#30451](bitcoin#30451) will enable `ccache` to work with macOS cross-compilation builds (i.e. they will no longer be registered as uncacheable). * After merging in [dash#6564](#6564), existing caches resulted in across-the-board build failures ([build](https://github.com/dashpay/dash/actions/runs/13290871849/job/37113644102#step:7:2768), [build](https://github.com/dashpay/dash/actions/runs/13290791962/job/37113418275#step:7:2940), [build](https://github.com/dashpay/dash/actions/runs/13289932901/job/37109498600#step:7:2177)) due to differing compiler versions (GCC 13 build on GCC 11 based cache) and differing glibc versions (2.39 build vs 2.35 cache) due to the change from `jammy` (on which the caches were built) to `noble` (the new base image for CI). To prevent this from occurring in the future, we will add CI's [`Dockerfile`](https://github.com/dashpay/dash/blob/265d9b5f9f5af07ff9da3d6fcc318b783d18caea/contrib/containers/ci/Dockerfile) to the `hashFiles()` used to generate the root of the cache key to ensure that the cache is discarded if it is modified. * `runner.os` doesn't help us as it tells us what platform the _runner_ is on and is independent from what the base image in the container is using, we only care about the latter. * The SDKs were cached separately because * The sources cache is shared between variants and the fastest runner (usually a Linux runner) will set the cache and as the download and extraction of the macOS SDK is only done when expected to target macOS, the cache associated with the shared key will not have the requisite SDKs. Then when that cache key is used to pull the sources cache by the macOS build, it will succeed due to a cache hit and then fail building because the compiler cannot locate the SDK. * The build cache is expected to have a higher churn rate as it is tied to the platform, which will trigger more than necessary downloads of the SDK. * If placed here, the cache would need to be re-evaluated if the version of Xcode has been bumped, which involves modifying `depends/hosts/darwin.mk`. Meaning, that file needs to be included when computing `hashFiles()`. But this also would result in unnecessary cache misses for all the platforms that _aren't_ macOS (because the structure of the build key is the same among all variants). ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 106500c PastaPastaPasta: utACK 106500c; gonna do CI on my repo, and push an empty commit and ensure caches are working as expected Tree-SHA512: b51608d8791a0d87b6d43388d4173e31123fd62fc3ae346907102504a454133e7b862bf3be784a3610ebd2b816b2aaab8c1d4a2f33a4400cc6cf7bce380e9adf
3 participants
Motivation
Since dash#6389, the minimum supported version of GCC has been 11.1 or later. Our current base, Ubuntu 22.04 LTS (
jammy) ships with GCC 11.2 (source). The cross-compilation package forarm-linux-gnueabihfis GCC 11.2 as well (source). Unfortunately, this isn't the case formingw-w64-x86-64, which ships with GCC 10.3 (source).So far, an workaround was utilized to allow GCC 10.3 to pass muster upstream through bitcoin#28379 (source). Dash Core's enablement of C++20 experimental support in dash#4600 was relatively even more permissive (source).
Though since then, enforcement of those newer minimum requirements through backports like bitcoin#30228 are frustrated by the version of GCC shipped for Windows cross-compilation.
This can be resolved by bumping the image to the next available LTS release, Ubuntu 24.04 (
noble), which ships with GCC 13.2 natively (source), for ARM Linux (source) and AMD64 Windows (source), moreover, it is acknowledged as a de facto lowest supported distro needed for Windows cross compilation by bitcoin#30580.Skipping the enforcement of newer minimum requirements is inadvisable, primarily because it will eventually conflict with upcoming code changes (comment) like dash#6378.
Additional Information
Dependency for backport: merge bitcoin#24460, #27672, #27766, #28065, #30228, partial bitcoin#28349, #28579 (drop c++17 support) #6380
A default unprivileged user named
ubuntuwas introduced in Ubuntu 22.10 (source) with UID1000and GID1000.We allow specifying UID and GID to ensure they match with the ownership of directories expected to be mounted to avoid permissions issues (especially on platforms like macOS where the user can have a UID of
501and GID of20).ubuntuuser and theubuntugroup will have its UID and GID updated. This is a no-op if defaults are selected.dash), it will be additionally renamed fromubuntu.dash(build) and it results in a runner failure.Due to a conflict between
gcc-multilibandgcc-arm-linux-gnueabihf, installation of the latter will result in the uninstallation of the former. This has been documented behavior for a while now (source) and continues till today (see below).Error message:
Since dash#5372, we have stopped supporting i686 and dropped support for 32-bit Windows even earlier. There doesn't seem to be much reason to keep
gcc-multilibaround, especially since it cannot be around and has been silently uninstalled for a while now, so the package has now been dropped.wine32setup (source). Support has improved furthermore in Wine 9.0 (source), which is available innoble(source). This allows us to drop the i386 conditional altogether.libncurses5, while a valid package onjammy(source), is not in any future version, includingnoble(source, error page) thoughlibncurses5-devcontinues to remain a valid package (source) and remains used as a Python dependency (source). As a result,libncurses5has been dropped.A bump in distro base also causes a bump in glibc version, which causes zlib, a Qt dependency to fail a compile-time check (build), this is avoided by backporting bitcoin#29985.
Due to QTBUG-133261, UBSan will raise a
float-cast-overfloworiginating fromqRound(double)(build), as of this writing, a fix is currently underway (source) but it is unclear when it would be available in the next Qt 5.15 revision.As this doesn't seem to be a regression (source) despite being undesirable behavior, we can suppress the alarm.
Checklist