-
Notifications
You must be signed in to change notification settings - Fork 409
Update Clang Format Dockerfile to work with docker and rootless podman #7939
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
base: master
Are you sure you want to change the base?
Conversation
@keithc-ca @jdmpapin could you please review (since you guys had issues with the original Dockerfile). I tested this on both docker (which uses root because of the daemon) as well as rootless podman on RHEL 8.10, and both seemed to work OOTB. |
Using
I think the issue is that |
Ah, that error isn't because it's too old, but because whoever built We can suppress that output, but then we risk losing out other stderr output from clang-format. |
I think something like this (untested) snippet could do it without losing other output:
|
What's the objection to building upon ubuntu as I suggested? |
I didn't voice an objection; I was clarifying that the output
has nothing to do with the version of ubuntu but that |
On the contrary, if the image is built upon Ubuntu, that message does not appear. I interpret that to mean |
A container will not delegate to the host with respect to finding libraries. If the library does not exist inside the container, a program that wants to load the library will fail to load the library. The fact that we're seeing the message
at all means that As mentioned before, and pointing to the stack overflow link I listed above, the issue here is that the library That's it. Building with Now, I don't have a problem using ubuntu as a base, but I want to make clear what the issue is here is all. |
I wasn't expecting references to leak out to the host, and you're right, in the UBI variant the library is at # ls -l /lib64/libtinfo.so*
lrwxrwxrwx. 1 root root 15 Aug 15 2023 /lib64/libtinfo.so.5 -> libtinfo.so.5.9
-rwxr-xr-x. 1 root root 178936 Aug 15 2023 /lib64/libtinfo.so.5.9
lrwxrwxrwx. 1 root root 15 Aug 15 2023 /lib64/libtinfo.so.6 -> libtinfo.so.6.1
-rwxr-xr-x. 1 root root 187552 Aug 15 2023 /lib64/libtinfo.so.6.1 In my variant based on ubuntu, that library is in # ls -l /lib/x86_64-linux-gnu/libtinfo*
lrwxrwxrwx. 1 root root 15 May 16 2023 /lib/x86_64-linux-gnu/libtinfo.so.5 -> libtinfo.so.5.9
-rw-r--r--. 1 root root 183824 May 16 2023 /lib/x86_64-linux-gnu/libtinfo.so.5.9
lrwxrwxrwx. 1 root root 15 May 16 2023 /lib/x86_64-linux-gnu/libtinfo.so.6 -> libtinfo.so.6.2
-rw-r--r--. 1 root root 192032 May 16 2023 /lib/x86_64-linux-gnu/libtinfo.so.6.2 I don't know why the UBI-based image is broken, or whether the date of the library is significant (the Ubuntu version is a little older), but it seems sensible to use Ubuntu when the archive we're using has |
I used Claude to update the Dockerfile to use diff --git a/buildenv/jenkins/clang_format/Dockerfile b/buildenv/jenkins/clang_format/Dockerfile
index 94eb32fb50..278b125999 100644
--- a/buildenv/jenkins/clang_format/Dockerfile
+++ b/buildenv/jenkins/clang_format/Dockerfile
@@ -22,21 +22,26 @@
# Some sections generated by Claude 3.7 Sonnet (version 20250219)
-FROM registry.access.redhat.com/ubi8/ubi-minimal:latest
+FROM ubuntu:20.04
+
+# Avoid interactive prompts during package installation
+ENV DEBIAN_FRONTEND=noninteractive
# Create a non-root user and group
-RUN microdnf install -y shadow-utils \
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ ca-certificates \
+ && rm -rf /var/lib/apt/lists/* \
&& groupadd -g 1000 clanguser \
- && useradd -u 1000 -g clanguser -m -s /bin/bash clanguser \
- && microdnf remove -y shadow-utils \
- && microdnf clean all
+ && useradd -u 1000 -g clanguser -m -s /bin/bash clanguser
# Install required packages
-RUN set -eux; \
- microdnf install -y \
- wget tar xz ncurses-compat-libs \
- && microdnf update -y \
- && microdnf clean all
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ wget \
+ tar \
+ xz-utils \
+ libncurses5 \
+ && apt-get clean \
+ && rm -rf /var/lib/apt/lists/*
# Create directories with appropriate permissions
RUN mkdir -p /opt/clang-tools /src \
@@ -48,8 +53,10 @@ RUN cd /tmp \
&& tar -xvf clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04.tar.xz --no-same-owner \
&& cp clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04/bin/clang-format /opt/clang-tools/ \
&& rm -rf clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04* \
- && microdnf remove -y wget tar xz \
- && microdnf clean all
+ && apt-get purge -y wget \
+ && apt-get autoremove -y \
+ && apt-get clean \
+ && rm -rf /var/lib/apt/lists/*
# Set up environment
ENV PATH=/opt/clang-tools:$PATH This works again on both Docker and rootless podman (RHEL 8.10). However, we do risk running into Dockerhub's pull limit. We could use Amazon's ECR to get around this. From an OSS pov, the RH Registry is probably still the best option, which would be a case to stick with UBI. Do you have any opinion on this consideration? @0xdaryl @mstoodle: could you provide your thoughts from the project lead pov. |
The complaint about libtinfo.so is occurring on Jenkins now; see https://ci.eclipse.org/omr/job/PullRequest-Clang-Format-Check/257:
I would hope that this change makes that stop. |
Yes, this was already known. Irwin said above:
|
The complaint has always been there. I explicitly called it out in the code formatting PR #7846 (comment). Switching to ubuntu will remove that error line, but as I said, we have to contend with dockerhub pull limitations. So we need to decide on the trade off of that message against dockerhub. I'm deferring that decision to the project leads. |
@0xdaryl @mstoodle reminder ping for the question of ubuntu vs ubi base image, namely the question in #7939 (comment). |
I spoke with @dsouzai and @AdamBrousseau offline about the various container registry alternatives. I think it makes sense to stick with the UBI images from the RH registry given the restrictions many of the expected consumers/users of this Dockerfile have with DockerHub. |
Signed-off-by: Irwin D'Souza <[email protected]> Co-authored by Claude 3.7 Sonnet (version 20250219)
Signed-off-by: Irwin D'Souza <[email protected]> Co-authored by Claude 3.7 Sonnet (version 20250219)
908e1ae
to
d69f8e3
Compare
@keithc-ca d69f8e3 suppresses the libtinfo error message. Good for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll repeat (perhaps for the last time) my concern about mixing an archive clearly meant for Ubuntu with the ubi8/ubi-minimal environment: I think it's just asking for trouble.
# | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one such line is sufficient.
# Some sections generated by Claude 3.7 Sonnet (version 20250219) | ||
|
||
# Wrapper script for clang-format that filters out the libtinfo warning | ||
# while preserving other error messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use proper punctuation throughout (e.g. finish each sentence with a period).
output=$(/opt/clang-tools/clang-format.real "$@" 2> >(grep -v "libtinfo.so.5: no version information available" >&2)) | ||
exit_code=$? | ||
|
||
# Output the result | ||
echo "$output" | ||
|
||
# Return the original exit code | ||
exit $exit_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me that capturing output in a shell variable is a good idea. I think, as is, it's subject to space interpolation which I don't think we want. It should also be unnecessary.
&& tar -xvf clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04.tar.xz --no-same-owner \ | ||
&& cp clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04/bin/clang-format /opt/clang-tools/clang-format.real \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to unpack everything else from the archive.
No description provided.