[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

Fri Aug 21 14:53:24 PDT 2020

aganea added subscribers: amccarth, dblaikie.
aganea added a comment.

In D85998#2231001 <https://reviews.llvm.org/D85998#2231001>, @zahen wrote:

> The build system strives to be deterministic

When you say build system, you mean MSBuild? Other? For example, Fastbuild purposely calls `CreateProcess` with an empty environment block (not null) to avoid determinism issues. Is that your case? We are also using `-nostdinc` to avoid any compiler-generated include paths, even in cases where you need to repro something in a VS cmd shell.

The patch looks good otherwise, modulo the comments. + @amccarth @dblaikie for more opinions.

================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:74
+   if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+    Path = A->getValue();
+    VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
----------------
zahen wrote:
> Deliberately not validating the input.  The primary motivation is to prevent unnecessary file and registry access.
I would add what you said above as a comment. It is interesting for future readers of this code, and avoids digging the history for intent.

================
Comment at: clang/test/Driver/cl-options.c:686
+// vctoolsdir is handled by the driver; just check that we don't error. Pass -c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+
----------------
Check that we're not detecting a local installation, and that we fallback to the default triple 19.11, ie.
```
// RUN: %clang_cl ... -vctoolsdir "" -### | FileCheck %s --check-prefix VCTOOLSDIR
// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
```

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

https://reviews.llvm.org/D85998