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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a subscriber: Meinersbur.
awarzynski added a comment.

Good morning @PeteSteinfeld , thank you for making this documentation so much clearer! I have a couple of small suggestions (these are not blockers for me).

**out-of-tree **vs **standalone**
Have you considered switching to "standalone builds" instead of "out-of-tree builds"? That would be more:

- consistent with the actual implementation/naming in the CMake script <https://github.com/llvm/llvm-project/blob/main/flang/CMakeLists.txt#L27>,
- accurate, i.e. Flang is always in-tree  these days (i.e. inside `llvm-project`).

I guess that "out-of-tree" was accurate/descriptive enough before "f18" was merged into "llvm-project". But these days Flang is always in tree. Also, "standalone" would be more consistent with other sub-projects.

**CMake flags**
On a different note, would it make sense to trim this set of CMake flags?

  cmake \
    -G Ninja \
    ../llvm \
    -DCMAKE_BUILD_TYPE=Release \
    -DLLVM_LIT_ARGS=-v \
    -DFLANG_ENABLE_WERROR=On \
    -DCMAKE_CXX_STANDARD=17 \
    -DLLVM_ENABLE_PROJECTS="clang;mlir;flang;compiler-rt" \
    -DLLVM_BUILD_TOOLS=On \
    -DLLVM_INSTALL_UTILS=On \
    -DLLVM_TARGETS_TO_BUILD=host \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DCMAKE_CXX_LINK_FLAGS="-Wl,-rpath,$LD_LIBRARY_PATH" \
    -DCMAKE_INSTALL_PREFIX=$INSTALLDIR

Most developers want to run `ninja check-flang` and for that the following should be sufficient:

  make \
    -G Ninja \
    ../llvm \
    -DCMAKE_BUILD_TYPE=Release \
    -DFLANG_ENABLE_WERROR=On \
    -DLLVM_ENABLE_PROJECTS="clang;mlir;flang" \
    -DLLVM_TARGETS_TO_BUILD=host \
    -DLLVM_ENABLE_ASSERTIONS=ON

Other flags are only really needed for "out-of-tree"/"standalone" builds (in order to install or build dependencies) or to make the LIT output more verbose.

I will also add @Meinersbur to the list of reviewers as he's very familiar with LLVM's build system.



================
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.
+
----------------
Is `compiler-rt` really needed? I never include it myself.


================
Comment at: flang/README.md:59
+execute the following commands, to do the build:
+```
+INSTALLDIR=`pwd`/install
----------------
You could shorten the text by adding this in the bash snippet:
```
git clone https://github.com/llvm/llvm-project.git
cd llvm-project
```


================
Comment at: flang/README.md:59
+execute the following commands, to do the build:
+```
+INSTALLDIR=`pwd`/install
----------------
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.


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


================
Comment at: flang/README.md:110
+
+cmake \
+  -G Ninja \
----------------
No `compiler-rt` here would suggest that it's not required at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116566