[libc++][CI] Improves Dockerfile

[Mordante]

Summary

• Remove temporary apt files
• Removes LLVM16 TODO

[philnik]

libcxx/utils/ci/Dockerfile

78–79

Could you align all the backslashes to 120 columns to make them less annoying?

83–87

[Mordante]

libcxx/utils/ci/Dockerfile

83–87

I like to keep the comments for clarity.

[Mordante]

Addresses review comments.

[philnik]

libcxx/utils/ci/Dockerfile

83–87

I really don't see how describing what the lines do on-by-one help in any way. They just make it harder to read IMO.

[ldionne]

I like that we are removing the LLVM 16 todo, but I am not sold on the other changes to the Dockerfile. I'd like to understand your rationale for them.

libcxx/utils/ci/Dockerfile

78–79

Isn't it better to have more layers? That way you get more layers of caching and can potentially re-download less when you update the image, no?

[ldionne]

As discussed, I am not sure this actually solves a problem we encounter -- our image isn't that large and I don't think the image download times have been a problem. If that's incorrect, please LMK since that changes my opinion about this patch.

But assuming that we're not really gaining something noticeable in terms of speed/size, IMO this patch is a small net loss since it's a bit less readable than what we had before. This is a part of our CI infrastructure and IMO we should strive to keep it as simple as possible to keep the barrier for people maintaining it as low as possible -- to me that has more value than a bit more size in the image.

[ldionne]

FWIW I'd be fine with cleaning up the Debian packages in a separate RUN command. This won't reduce the size we have to download, but would make the image itself a bit smaller, I guess.

[Mordante]

Thanks for the review. I will basically revert these changes and only remove the unneeded package and the apt removal.

libcxx/utils/ci/Dockerfile

83–87

As discussed with Louis this entire hunk will be reverted.

[Mordante]

Addresses review comments.

libcxx/utils/ci/Dockerfile

 RUN apt-get update && apt-get install -y clang-format-$(($LLVM_HEAD_VERSION - 2)) clang-format-$(($LLVM_HEAD_VERSION - 1))
 
 # Install clang-tidy
 RUN apt-get update && apt-get install -y clang-tidy-$(($LLVM_HEAD_VERSION - 2)) clang-tidy-$((LLVM_HEAD_VERSION - 1))
 
 # Install llvm-dev and libclang-dev to compile custom clang-tidy checks
 RUN apt-get update && apt-get install -y llvm-$(($LLVM_HEAD_VERSION - 1))-dev     llvm-$(($LLVM_HEAD_VERSION - 2))-dev \
                                      libclang-$(($LLVM_HEAD_VERSION - 1))-dev libclang-$(($LLVM_HEAD_VERSION - 2))-dev
 
-# TODO LLVM16 Don't install llvm-16-dev explicitly
-RUN apt-get update && apt-get install -y llvm-16-dev libclang-16-dev
-
 # Install the most recent GCC, like clang install the previous version as a transition.
 ENV GCC_LATEST_VERSION=12
 RUN apt-get update && apt install -y gcc-$((GCC_LATEST_VERSION - 1)) g++-$((GCC_LATEST_VERSION - 1))
 RUN apt-get update && apt install -y gcc-$GCC_LATEST_VERSION g++-$GCC_LATEST_VERSION
 
+# Remove all, no longer needed, apt data
+RUN rm -rf /var/lib/apt/lists/*
+
 # Install a recent CMake
 RUN wget https://github.com/Kitware/CMake/releases/download/v3.21.1/cmake-3.21.1-linux-x86_64.sh -O /tmp/install-cmake.sh
 RUN bash /tmp/install-cmake.sh --prefix=/usr --exclude-subdir --skip-license
 RUN rm /tmp/install-cmake.sh
 
 # Change the user to a non-root user, since some of the libc++ tests
 # (e.g. filesystem) require running as non-root. Also setup passwordless sudo.
 RUN apt-get update && apt-get install -y sudo
 RUN echo "ALL ALL = (ALL) NOPASSWD: ALL" >> /etc/sudoers