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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski accepted this revision.
awarzynski added a comment.
This revision is now accepted and ready to land.

LGTM, thank you for this!



================
Comment at: flang/README.md:54
+Building flang in tree means building flang along with all of the projects on
+which it depends.  These projects include mlir, clang, flang, and compiler-rt.  Note that compiler-rt is only needed to access libraries that support 16 bit floating point numbers.  It's not needed to run the automated tests.
+
----------------
[nit] Would you mind adding some line feeds here before merging? People usually tweak their editors for narrower lines.


================
Comment at: flang/README.md:130
+```
+Note that for Clang and MLIR you use the installation directory ($base/install) and for LLVM you use the build directory (`$base/build`).   This is not a typo in the script.  Rather, it is because running the tests requires the GTest infrastructure which is only available in the LLVM build area.  The build also requires the `AddClang.cmake` script from Clang, which is only available in the install area.
+
----------------
[nit] Would you mind adding some line feeds here before merging? People usually tweak their editors for narrower lines.


================
Comment at: flang/README.md:123-125
+  -DLLVM_DIR=$base/build/lib/cmake/llvm \
+  -DCLANG_DIR=$base/install/lib/cmake/clang \
+  -DMLIR_DIR=$base/install/lib/cmake/mlir \
----------------
PeteSteinfeld wrote:
> awarzynski wrote:
> > It's a bit confusing that for LLVM it's `$base/build/` and otherwise it's `$base/install/`. If I recall correctly, you need:
> > * build directory for LLVM, because that's the only location where GTest is available (required for unit tests)
> > * install directory for Clang, because that's the only location were `AddClang.cmake` script is available (required for Clang configuration)
> > 
> > Fixing this would be nice, but that's totally out of scope here! Perhaps you could add a note outside this code snippet to highlight this inconsistency, 
> > > "Note that for Clang and MLIR you will use the installation directory (`$base/install`) and for LLVM you will need to build directory (`$base/build`). You can use the installation directory for all three sub-projects if you don't require the unit tests (which are configured by LLVM and are only available in the build directory)."
> > 
> > Otherwise this inconsistency is confusing and people might think that it's a typo. Or just miss it altogether!
> You're analysis is correct.
Thanks for the update! I believe that with https://reviews.llvm.org/D116731 we should be able to use `$base/build` for all sub-projects. We can re-visit this document once that's merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116566