[PATCH] D108513: [docs] Update Getting Started with Visual Studio guide

Tue Aug 24 11:25:15 PDT 2021

yaron.keren marked 15 inline comments as done.
yaron.keren added inline comments.

================
Comment at: llvm/docs/GettingStartedVS.rst:43
+Object files, libraries and executables consume approximately 5GB in
+Release mode and much more in Debug mode. SSD drive is recommended.
+
----------------
aaron.ballman wrote:
> If we're going to recommend hardware, should we also recommend a significant amount of RAM?
OK

================
Comment at: llvm/docs/GettingStartedVS.rst:48
 --------
-You will need Visual Studio 2017 or higher, with the latest Update installed.
+You will need `Visual Studio <https://visualstudio.microsoft.com/>`_ 2017 or
+higher, with the latest Update installed.
----------------
kuhnel wrote:
> Nit: Maybe add a comment that the "Community Edition" is enough.
ok

================
Comment at: llvm/docs/GettingStartedVS.rst:55
-<http://www.python.org/>`_. Version 3.6 and newer are known to work. You will
-need `GnuWin32 <http://gnuwin32.sourceforge.net/>`_ tools, too.

----------------
mstorsjo wrote:
> aaron.ballman wrote:
> > mstorsjo wrote:
> > > amccarth wrote:
> > > > aaron.ballman wrote:
> > > > > I'm a bit worried about losing this information; I didn't have luck skipping GnuWin32 the last time I tried (about a year ago), so I don't think just Python is sufficient. WDYT?
> > > > The git-provided versions of common unix tools have been sufficient for me (except there's no make in git).  The git ones are also newer than the GnuWin32 ones, and a couple of them behave slightly differently.
> > > > 
> > > > The only reason I still I install GnuWin32 is because lldb tests require make.  That said, I make sure that GnuWin32 comes _after_ git in my PATH.  Otherwise, other things go wrong.  GnuWin32 is old and unmaintained.  I don't think we can expect Windows developers to continue to rely on it.
> > > > 
> > > > I'm not against mentioning it as a possibility, but I don't think it belongs in the mainline "Getting Started" recommendations.
> > > Also FYI, regarding GnuWin32 vs the git unix tools - the llvm lit tests also have a provision where they try to locate the git unix tools and add them to the path, but this isn't hooked up when running other testsuites (like for libcxx). See https://github.com/llvm/llvm-project/blob/main/llvm/utils/lit/lit/llvm/config.py#L29-L35.
> > If the git tools are sufficient, that's fantastic news (I've been hoping we can get rid of this insecure and long-unsupported requirement for years)! When I last tried, they weren't sufficient; I recall some Clang lit tests were failing because of missing utilities like `sed`. But if you think they're now sufficient, perhaps we should document that more clearly as a need for the testing system? (Also, perhaps the issue is that Git for Windows that comes with VS 2019 may be different from what you download manually, and if so, that would be useful to tell people if it actually matters to the testing system.)
> At least the pre-merge test bot runs without GnuWin32 now, but it manually adds the `c:\Program Files\Git\usr\bin` directory to the path instead. The llvm lit config that I linked does look for a path to something similar via the registry, I guess that doesn't find git bundled with Visual Studio (and I'm not sure if that bundled the same unix utilities in the same way).
The Visual Studio installer installs a slightly older git (2.31.1) than the standalone installer from the web site (2.33). Otherwise identical and (unlike cmake) installed into the same install location (`c:\Program Files\Git\`).

The bash utilities installed in `c:\Program Files\Git\usr\bin\` (from either version of Git for Windows) pass regression tests for llvm & clang pass. Nothing besides the documented here is required.

llvm-lit config.py indeed can find Git for Windows utilities even if not on the PATH by looking in the registry. This is a very nice "just works" functionality to share with other llvm projects.

I had not tested projects of LLVM other than llvm and clang - this is the scope of a "Getting Started" page ?

================
Comment at: llvm/docs/GettingStartedVS.rst:58
+install Python with Visual Studio 2019, from the Microsoft store or from
+the `Python web site <http://www.python.org/>`_. We recommend the later since it
+allows you to to adjust installation options.
----------------
aaron.ballman wrote:
> 
Thanks for catching this!

================
Comment at: llvm/docs/GettingStartedVS.rst:110

-   * With git access:
+11. Finally, configure LLVM using CMake:

----------------
aaron.ballman wrote:
> kuhnel wrote:
> > Nit: Maybe call out, that you're only building clang here and users need to configure the other projects as needed, e.g. as explained in https://llvm.org/docs/CMake.html#frequently-used-llvm-related-variables
> Before getting to "finally", should we explain what needs installation for tests to work (like GnuWin32)?
GnuWin32 functionality is replaced by the bash tools in Git for Windows (in `c:\Program Files\Git\usr\bin\`)

================
Comment at: llvm/docs/GettingStartedVS.rst:110

-   * With git access:
+11. Finally, configure LLVM using CMake:

----------------
yaron.keren wrote:
> aaron.ballman wrote:
> > kuhnel wrote:
> > > Nit: Maybe call out, that you're only building clang here and users need to configure the other projects as needed, e.g. as explained in https://llvm.org/docs/CMake.html#frequently-used-llvm-related-variables
> > Before getting to "finally", should we explain what needs installation for tests to work (like GnuWin32)?
> GnuWin32 functionality is replaced by the bash tools in Git for Windows (in `c:\Program Files\Git\usr\bin\`)
Clarified at the page that the scope is llvm&clang only, to keep this simple. Refered to the CMake page.

================
Comment at: llvm/docs/GettingStartedVS.rst:114

-      1. ``cd <where-you-want-llvm-to-live>``
-      2. ``git clone https://github.com/llvm/llvm-project.git llvm``
-      3. ``cd llvm``
+       cmake -S llvm\llvm -B build -DLLVM_ENABLE_PROJECTS=clang -DLLVM_TARGETS_TO_BUILD=X86 -Thost=x64
+       exit
----------------
amccarth wrote:
> kuhnel wrote:
> > I'm not familiar with using CMake with the Visual Studio build system, but don't you need to set the output build system with `cmake -G "Visual Studio 17 2022`? Or is that set automatically?
> That's an interesting question.  I've never actually built from the Visual Studio IDE.  I use CMake to generate solution and project files for browsing, editing, and debugging.  But I also have CMake generate a ninja solution and actually build from the command line.  In both cases, I explicitly specify the generator.  If we're not recommending ninja for builds, then maybe we don't need to mention how to install it.
On Windows cmake defaults to the latest Visual Studio (and on Linux to Unix Makefiles).

================
Comment at: llvm/docs/GettingStartedVS.rst:145
+   Studio configuration is **Debug** which is slow and generates a huge amount
+   of debug information on disk. For now, we recommend selecting **Release**
+   configuration for the LLVM project. It is possible to change compiler
----------------
kuhnel wrote:
> Why not Release with Debug info? This gives you much more information when debugging problems?
On my system Release build is 1.8GB whereas RelWithDebInfo build is 16GB. Will explain this.

================
Comment at: llvm/docs/GettingStartedVS.rst:177

-7. Build the LLVM Suite:
+   choco install -y ninja git cmake gnuwin python3
+   pip3 install psutil
----------------
kuhnel wrote:
> I guess we should also update these instructions, based on your new tool recommendations. 
OK

================
Comment at: llvm/docs/GettingStartedVS.rst:250

-      C:\..> clang -c hello.c -emit-llvm -o hello.bc
+      c:\..> clang -c hello.c -emit-llvm -o hello.bc

----------------
aaron.ballman wrote:
> Not certain why we need to churn these examples (FWIW, capital drive letters are still quite common).
Yes indeed this page should be Visual Studio specific - removed!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108513