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

Mon Jun 19 02:17:05 PDT 2017

ilya-biryukov added a comment.

Thanks for the comments.

> - 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

We hope the added value would be more significant after we improve the scripts and address your other comments (mostly, configurability).

> - 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.

I would advocate for an easier interface to the build script, even if it would add some maintenance costs (that said, I'm ready to maintain it until it's trivial).
I think having a script, accepting parameters like 'make a 2-stage bootstrap and install lld+clang)', is a better interface than 'checkout projects cfe, lld, pass arguments "-X -Y -Z" to cmake'. Keeping an interface to the build script simple (while also not letting the maintenance costs blow up) should be a priority here, IMO.

I've also requested this in the inline comment, but wanted to repeat myself here: could you elaborate more on your specific use-case? (i.e. what tools from LLVM do you want installed on your machine. only lld, compiled using 2-stage bootstrap and LTO?)

================
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
----------------
mehdi_amini wrote:
> Why does it install gcc? Why not apt-get install clang?
No specific reason, since we do 2-stage build and end up with "clang, compiled from clang" in the end, this shouldn't matter.

================
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.
----------------
mehdi_amini wrote:
> That gonna be quickly obsolete (if not already), docker allows to squash images now.
> 
Thanks, I wasn't aware of the '--squash' flag coming in Docker 1.13. 
It makes sense to go with a single image, instead of build+release, and suggest using a flag and a newer Docker version in the docs.
I'll add changes to incorporate that.

================
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"
+
----------------
mehdi_amini wrote:
> 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.
The maintenance cost is exactly the reason why we hard-coded the script to build only clang. It makes the script very easy. That said, I'm in favour of more configuration in the future, building clang is just a first iteration.

I can spend some time making the script configurable enough to cover your use-case too. Than we could iterate to make it even more configurable.
> I suspect everyone will have different needs, and we will end up with a very complicated script to handle all possibilities
Could you elaborate more on what exactly are 'only some targets to begin with'?

================
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"
+
----------------
mehdi_amini wrote:
> 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).
Thanks, will look into using it.

================
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 ---------===//
----------------
mehdi_amini wrote:
> 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?
Docker also requires all the files you ADD to the images to be under 'build context' (i.e. a dir you pass to docker build). Currently, the script passes a specific image directory (i.e. debian8 or nvidia-cuda) as a build content, therefore we need to have a script in each of those dirs.

I agree with you, though, duplicating the script is not ideal. I'll change the scripts to remove copies of `build_install_clang.sh`. Thanks for the comment.

https://reviews.llvm.org/D34197