[PATCH] D62040: [docs] Add new document on building distributions

[Wink Saville via Phabricator via llvm-commits]

winksaville added a comment.

A few suggestions to consider, but this is GREAT, thanks!



================
Comment at: llvm/docs/BuildingADistribution.rst:53
+  *BUILD_SHARED_LIBS* CMake option. That option exists for optimizing developer
+  workflow only. Due to design and implementation decisions in LLVM relies on
+  global data which can end up being duplicated across shared libraries
----------------
"Due to design and implemenation decisions in LLVM relies on" can be improved, maybe remove "in" and add a comma; "Due to design and implemenation decisions, LLVM relies on ..."


================
Comment at: llvm/docs/BuildingADistribution.rst:86
+generates a ``distribution`` target which builds all the components specified in
+the list. This is a convenience build target to allow building just the
+distributed pieces without needing to build all configured targets.
----------------
Is `distributation` target an `internal` target that shouldn't be used or a `convenience` target and might be used by the people building the distribution ? If the former consider saying "internal build target".


================
Comment at: llvm/docs/BuildingADistribution.rst:108
+set on the stage-2 compiler either using a CMake cache file, or by prefixing the
+option with *BOOTSTRAP_*.
+
----------------
Consider adding' with "BOOTSTRAP_*, see :doc:`AdvancedBuilds`.


================
Comment at: llvm/docs/BuildingADistribution.rst:112
+the simplest way to control the optimization level is by setting the
+*CMAKE_BUILD_TYPE* option. The main values of interest are ``Release`` or
+``RelWithDebInfo``. By default the ``Release`` option uses the ``-O3``
----------------
I suggest simplifying 110..112 to something like:

"The first and simplest way to control the optimization is using `CMAKE_BUILD_TYPE`. The main ..."


================
Comment at: llvm/docs/BuildingADistribution.rst:123
+the binaries in the distribution, but it will create much faster binaries. This
+option should not be used if your distribution includes static archives, as the
+objects inside the archive will be LLVM bitcode, which is not portable.
----------------
Consider changing "includes static **archives**" to "includes static **libraries**" as archives isn't used anywhere else.


================
Comment at: llvm/docs/BuildingADistribution.rst:155
+between the LLVM-based tools. This can be done by setting the following two
+CMake options to ``On``: *LLVM_BUILD_LLVM_DYLIB* and *LLVM_LINK_LLVM_DYLIB*.
+
----------------
Maybe add "see :doc:`CMake`"


================
Comment at: llvm/docs/BuildingADistribution.rst:188
+
+**LLVM_DYLIB_COMPONENTS**:STRING
+  This variable can be set to a semi-colon separated name of LLVM library
----------------
Consider moving to ":doc:`CMake`" and then added to the "already documented" list above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62040