[PATCH] D81571: [analyzer] SATest: Add initial docker infrastructure

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked 5 inline comments as done.
vsavchenko added inline comments.


================
Comment at: clang/utils/analyzer/Dockerfile:10
+
+# newer CMake is required by LLVM
+RUN wget -O - https://apt.kitware.com/keys/kitware-archive-latest.asc 2>/dev/null | gpg --dearmor - | tee /etc/apt/trusted.gpg.d/kitware.gpg >/dev/null
----------------
NoQ wrote:
> Ouch. I'm pretty bad with docker but is it possible to simply take a newer ubuntu?
This is the latest LTS version of Ubuntu.  And actually that was one of the reasons I migrated to Fedora in my later pre-macOS years, Ubuntu is pretty bad with maintaining newer versions of packages.


================
Comment at: clang/utils/analyzer/Dockerfile:15
+# test system dependencies
+RUN apt-get update && apt-get install -y \
+    git \
----------------
NoQ wrote:
> NoQ wrote:
> > Maybe put `apt-get update` on a separate line?
> > 
> > Also i think you mentioned offline that you want to freeze package versions here, can you add a FIXME?
> > can you add a FIXME?
> 
> Wait, you already did that in D81600, nvm.
It is actually one of the good practices for writing Docker files and related to the Docker's build cache.

It is even recommended to do every `apt`-related stuff on the same line.  I went with one decision in between:
https://forums.docker.com/t/dockerfile-run-apt-get-install-all-packages-at-once-or-one-by-one/17191


================
Comment at: clang/utils/analyzer/entrypoint.py:31
+
+CMAKE_COMMAND = "cmake -G Ninja -DCMAKE_BUILD_TYPE=Release " \
+    "-DCMAKE_INSTALL_PREFIX=/analyzer -DLLVM_TARGETS_TO_BUILD=X86 " \
----------------
NoQ wrote:
> `-DLLVM_ENABLE_ASSERTIONS=ON`???
I was thinking about adding a separate list of options specifically for building.  Assertions can significantly affect performance and I don't know if that should be a default.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81571/new/

https://reviews.llvm.org/D81571