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

[Ilya Biryukov via Phabricator via llvm-commits]

ilya-biryukov added a comment.

@mehdi_amini, this should hopefully address your comments.
Could you take a final look one more time?



================
Comment at: docs/Docker.rst:109
+(``llvm/utils/build_clang_image.sh``) that handles that. It has less parameters
+and be used like that:
+
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > ilya-biryukov wrote:
> > > > mehdi_amini wrote:
> > > > > 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?
> > > > It is indeed just a thin wrapper with a sole purpose of including a proper "recipe" to do a 2-stage clang build.
> > > > Is there any harm in including it? It's a very small and simple script. Do you find it confusing to have it beside the main script to build docker images?
> > > > 
> > > Adding 90 lines of script for something that is 3 lines of documentation that the user should copy/paste? Doesn't justify itself to me.
> > > 
> > > 
> > Also, as mentioned in a previous review: we don't have a script that is "build clang" today (other than the release script. This is an orthogonal concern to Docker so I'm against tying it to Docker here.
> Also mentioned in a previous review: the cmake preconfigured cache files: they are superior to a script IMO because the user can more easily reuse them (you can include and override for instance).
> 
I've removed the script and updated the docs accordingly. They don't mention the cmake preconfigured cache files, but I hope that's a minor thing that could be fixed in further commits.



https://reviews.llvm.org/D34197