Skip to content

Install via pip instead of apt #264

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.

Sign up for GitHub

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

Closed
dkaszews wants to merge 3 commits into from
Closed

Install via pip instead of apt #264

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38bee07
Install via pip instead of apt
dkaszews Nov 24, 2025
5e72917
Hybrid install
dkaszews Nov 25, 2025
94035f6
Revert hybrid install, handle inexact version
dkaszews Nov 25, 2025
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
2 changes: 2 additions & 0 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ jobs:
- 19
- 20
- 21
- 21.1.6
steps:
- uses: actions/checkout@v6
- name: Build and test the Docker image
Expand Down Expand Up @@ -71,6 +72,7 @@ jobs:
- 19
- 20
- 21
- 21.1.6
path:
- check: 'test/known_fail'
exclude: 'capital'
Expand Down
9 changes: 5 additions & 4 deletions check.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ format_diff() {
local_format="$(docker run \
--volume "$(pwd)":"$(pwd)" \
--workdir "$(pwd)" \
ghcr.io/jidicula/clang-format:"$CLANG_FORMAT_MAJOR_VERSION" \
ghcr.io/jidicula/clang-format:"$CLANG_FORMAT_VERSION" \
--dry-run \
--Werror \
--style=file \
Expand All @@ -34,7 +34,7 @@ format_diff() {
formatted="$(docker run \
--volume "$(pwd)":"$(pwd)" \
--workdir "$(pwd)" \
ghcr.io/jidicula/clang-format:"$CLANG_FORMAT_MAJOR_VERSION" \
ghcr.io/jidicula/clang-format:"$CLANG_FORMAT_VERSION" \
--style=file \
--fallback-style="$FALLBACK_STYLE" \
"${filepath}")"
Expand All @@ -55,7 +55,8 @@ format_diff() {
return 0
}

CLANG_FORMAT_MAJOR_VERSION="$1"
CLANG_FORMAT_VERSION="$1"
CLANG_FORMAT_MAJOR_VERSION=$(echo "$CLANG_FORMAT_VERSION" | cut -d. -f1)
CHECK_PATH="$2"
FALLBACK_STYLE="$3"
EXCLUDE_REGEX="$4"
Expand Down Expand Up @@ -91,7 +92,7 @@ exit_code=0
docker run \
--volume "$(pwd)":"$(pwd)" \
--workdir "$(pwd)" \
ghcr.io/jidicula/clang-format:"$CLANG_FORMAT_MAJOR_VERSION" --version
ghcr.io/jidicula/clang-format:"$CLANG_FORMAT_VERSION" --version

# All files improperly formatted will be printed to the output.
src_files=$(find "$CHECK_PATH" -name .git -prune -o -regextype posix-egrep -regex "$INCLUDE_REGEX" -print)
Expand Down
9 changes: 2 additions & 7 deletions clang-format-docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,8 @@ FROM ubuntu:${UBUNTU_VERSION}

ARG CLANG_FORMAT_VERSION

RUN apt-get update \
&& apt-get install --no-install-recommends -y \
clang-format-${CLANG_FORMAT_VERSION} \
# Clean cache
&& apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false \
&& apt-get clean -y && rm -rf /var/lib/apt/lists/* \
&& mv /usr/bin/clang-format-${CLANG_FORMAT_VERSION} /usr/bin/clang-format
RUN pip install "clang-format~=$CLANG_FORMAT_VERSION" \
&& mv -f "$(which clang-format)" /usr/bin/clang-format
Copy link

@ptsouchlos ptsouchlos Nov 24, 2025

Choose a reason for hiding this comment

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

You likely need to install pip and python first before being able to run pip. Not sure where you're seeing failures but this may be the cause.

Copy link
Author

@dkaszews dkaszews Nov 25, 2025

Choose a reason for hiding this comment

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

Ubuntu tends to come with those preinstalled, unless docker has a different, very barebones image. I see errors running the test CI in my own repo:

Run "/home/runner/work/clang-format-action/clang-format-action/./../clang-format-action/check.sh" "21.1.6" "test/known_pass" "llvm" "capital" ""
Unable to find image 'ghcr.io/jidicula/clang-format:21.1.6' locally

This path suggests to me that we don't use the Dockerfile directly but instead from some repo, likely to cache it? So would need @jidicula to deploy those first (once tested to work), maybe under a new base name to avoid conflicts. Though having to manually deploy them makes me wonder how to handle so many versions, perhaps do the ones currently present on pip plus prefixes, i.e. 21.1.6 => 21.1.6, 21.1, 21.

Copy link

@echoix echoix Nov 25, 2025

Choose a reason for hiding this comment

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

It's because it isn't built yet, as the PR isn't merged. And for ghcr.io, you have to authenticate yourself before when downloading from the registry. In GItHub Actions, it is done automatically with the GITHUB_TOKEN

Copy link
Author

@dkaszews dkaszews Nov 25, 2025

Choose a reason for hiding this comment

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

@echoix Doesn't that create sort of chicken-and-egg scenario? We can't run CI on new images before we merge the PR, but shouldn't merge the PR without checking the new scenarios? Or is it just an artifact of me running in a fork whose token does not match the image cache? In any way I'll try testing the new image locally to ensure it works before marking as ready for review.

Copy link

@echoix echoix Nov 25, 2025

Choose a reason for hiding this comment

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

For tests in CI like that, you could do it in two steps in the same job of a workflow file. First, build the image only tagging it with the commit hash or something generic, but don't push it!! Make sure the "load" option of docker/build-push-action is true for this.

Then, run the tests, using the commit hash or the tag used above.
Since the image was loaded to the local image store, it will be available.

Then, rerun the metadata action, with the full labels/tags wanted, and build the image again. Since nothing changed since the last build, the build cache will be used and the build will finish right away. On this build, you can specify to allow pushing to a registry.
That last step shouldn't be done in PRs. You don't want PRs for the general public to publish unreleased images under your image name.

Copy link
Author

@dkaszews dkaszews Nov 25, 2025

Choose a reason for hiding this comment

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

You don't want PRs for the general public to publish unreleased images under your image name.

I'll just make this my takeaway, the other stuff is a bit over my head 😄 . I'll do local testing then ask for someone to take over the merging once ready.

Copy link

@echoix echoix Nov 25, 2025

Choose a reason for hiding this comment

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

You would want something similar to this example:

https://docs.docker.com/build/ci/github-actions/test-before-push/

But using two calls of https://github.com/docker/metadata-action. It will do the sanitation of tags, and help make sure the info is added. It's way simpler to use it rather than not use it.

Copy link
Author

@dkaszews dkaszews Nov 25, 2025

Choose a reason for hiding this comment

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

@echoix I have tested the new dockerfile locally, everything works as expected. Would you care taking over this PR to do the image publishing part? I unfortunately don't have time to learn this, especially since as first-time contributor to this repo, I can't even run the CI without maintainer approving them every time.

Copy link
Author

@dkaszews dkaszews Nov 25, 2025

Choose a reason for hiding this comment

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

Also, from what I understand, we need to rename all mentions of ghcr.io/jidicula/clang-format:"$CLANG_FORMAT_VERSION" to ghcr.io/jidicula/clang-format-pip:"$CLANG_FORMAT_VERSION", but then to pass CI we also need to manually build and deploy all those images, which sounds like something only @jidicula themselves can do.


WORKDIR /

Expand Down