[PATCH] D34197: Added Dockerfiles to build clang from sources.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: docs/Docker.rst:109
+(``llvm/utils/build_clang_image.sh``) that handles that. It has less parameters
+and be used like that:
+
----------------
It is very confusing why we need three scripts. I especially don't understand the introduction of the build_clang_image.sh script here.
Can we just get rid of this third script that (after looking at the code) is a thing wrapper and just give the example invocation of the build_docker_image.sh that does that?


================
Comment at: utils/docker/build_docker_image.sh:19
+  usage=$(cat << EOF
+Usage: build_docker_image.sh [options] [-- [buildsciprt_args]...]
+
----------------
s/buildsciprt_args/buildscript_args/


================
Comment at: utils/docker/debian8/release/Dockerfile:21
+    apt-get install -y --no-install-recommends libstdc++-4.9-dev binutils && \
+    rm -rf /var/lib/apt/lists/*
----------------
I'd put the RUN line here *before* the add to clang.tar.gz so that we don't always rebuild this layer when creating a new clang.


================
Comment at: utils/docker/scripts/build_install_llvm.sh:128
+    "https://llvm.org/svn/llvm-project/$SVN_PROJECT/$LLVM_BRANCH" \
+    "$CLANG_BUILD_DIR/src/$LLVM_PROJECT"
+done
----------------
Isn't the checkout gonna fail if a project is specified multiple times?


https://reviews.llvm.org/D34197