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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini requested changes to this revision.
mehdi_amini added a comment.
This revision now requires changes to proceed.

I'm interested into seeing this landing! However I also have concerns right now:

- The immediate added value for LLVM I could see right now is low as is. It could be useful for newcomers to get an easy setup to build and play with LLVM. But for this we also need to include this in http://llvm.org/docs/GettingStarted.html
- The scripts are fairly trivial and as such can't be useful to everyone: but adding more options would require a careful design (see inline comment). This is also adding some maintenance burden on the community. I actually think we should try to have *less* hard-coded logic in the build_install_clang.sh script in favor of some arguments. For instance it could take the list of projects to checkout and the cmake argument for the invocation. This is the easiest way I can imagine to be able to support as much use-case as possible while keeping the maintenance of this script as low as possible.



================
Comment at: docs/Docker.rst:58
+
+- `build/` image is used to compile clang, it installs gcc and all build
+  dependencies of clang. After the build process is done, the build image will
----------------
Why does it install gcc? Why not apt-get install clang?


================
Comment at: docs/Docker.rst:162
+  Therefore, we strive to provide a very simple release image which only copies
+  compiled clang and does not do anything else.
----------------
That gonna be quickly obsolete (if not already), docker allows to squash images now.



================
Comment at: utils/docker/debian8/build/build_install_clang.sh:62
+svn co -q $SVN_REV_ARG "http://llvm.org/svn/llvm-project/cfe/$LLVM_BRANCH" \
+  "$CLANG_BUILD_DIR/llvm/tools/clang"
+
----------------
That's where it becomes to get interesting: why only cfe?
Why wouldn't we package libc++, compiler-rt, lld, lldb, etc.
I understand you may not care, or that it even may be a burden (extra size), but as LLVM developers it isn't clear to me why we wouldn't have by default everything included.

It also seems to me that this script `build_install_clang.sh` is `yet another build script` to maintain while we already have some in the repo.

I suspect everyone will have different needs, and we will end up with a very complicated script to handle all possibilities (I for one am interested into using this, but with a very different config: bootstrap+LTO+lld+only some targets to begin with).

We really need more thoughts around this.


================
Comment at: utils/docker/debian8/build/build_install_clang.sh:81
+      -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX="$CLANG_INSTALL_DIR" \
+      "$CLANG_BUILD_DIR/llvm"
+
----------------
LLVM has a bootstrap process in a single cmake invocation, and can even build LLD during stage 1 and use it to link stage 2 (with LTO for example).


================
Comment at: utils/docker/nvidia-cuda/build/build_install_clang.sh:1
+#!/usr/bin/env bash
+#===- llvm/utils/docker/nvidia-cuda/build/build_install_clang.sh ---------===//
----------------
ilya-biryukov wrote:
> klimek wrote:
> > Are these build_install_clang.sh actually different at all?
> There're all the same because Docker doesn't allow symlinked files to be added into docker images.
> Added explanation for this in the docs/Docker.rst
I don't understand? 
It does not seem OK to me to duplicate this. Can't you pass a relative path to this file and store it in a common directory?


https://reviews.llvm.org/D34197