[PATCH] D116566: [flang] Fix the documentation on how to build flang

[Pete Steinfeld via Phabricator via llvm-commits]

PeteSteinfeld added a comment.

@awarzynski, thanks for the great feedback.

I like your suggestion of using "standalone".  I'll change it.

With respect to invoking CMake and its command line, I started by spending time finding the minimal set of options that would result in a correct, testable build.

I agree that we should minimize the number CMake flags.  I'll do it.  But I personally always use the verbose output of "lit".  It's much more useful, so I think it should be included.



================
Comment at: flang/README.md:54
+Building flang in tree means building flang along with all of the projects on
+which it depends.  These include llvm, mlir, clang, and compiler-rt.
+
----------------
awarzynski wrote:
> Is `compiler-rt` really needed? I never include it myself.
We recently found that it's needed to run executable tests in fir-dev.  Including it now will avoid needing to add it later in the instructions.


================
Comment at: flang/README.md:59
+execute the following commands, to do the build:
+```
+INSTALLDIR=`pwd`/install
----------------
awarzynski wrote:
> awarzynski wrote:
> > You could shorten the text by adding this in the bash snippet:
> > ```
> > git clone https://github.com/llvm/llvm-project.git
> > cd llvm-project
> > ```
> This will add syntax highlighting. Similar comment for other code snippets in this document.
Good suggestion about giving specific instructions for `git clone`.  But I think it's best to separate the cloning from the rest of the build commands.

Thanks for the tip about the bash highlighting.  You're a genius!


================
Comment at: flang/README.md:74
+  -DFLANG_ENABLE_WERROR=On \
+  -DCMAKE_CXX_STANDARD=17 \
+  -DLLVM_ENABLE_PROJECTS="clang;mlir;flang;compiler-rt" \
----------------
awarzynski wrote:
> You can skip this, it is set in the CMake build: https://github.com/llvm/llvm-project/blob/main/flang/CMakeLists.txt#L6
Thanks.  Will do.


================
Comment at: flang/README.md:110
+
+cmake \
+  -G Ninja \
----------------
awarzynski wrote:
> No `compiler-rt` here would suggest that it's not required at all.
As I mentioned earlier, it's needed for testing flang compiled executables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116566