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

[Zachary Henkel via Phabricator via cfe-commits]

zahen marked an inline comment as done.
zahen added a comment.

@hans the explicit path must be declared, all exes and dlls.  The unexpected probes for link.exe when invoking clang-cl.exe when it isn't actually going to be invoked are what we want to avoid.



================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:73
+                              MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.
----------------
hans wrote:
> What will the error look like if the user sets this but gets it wrong? I'm sympathetic to not wanting to validate it, but if it's wrong it would be nice if the error isn't too cryptic.
```
D:\>clang-cl -vctoolsdir fake main.cpp
clang-cl: error: unable to execute command: program not executable
clang-cl: error: linker command failed with exit code 1 (use -v to see invocation)
```


================
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
+
----------------
hans wrote:
> aganea wrote:
> > 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"
> > ```
> Maybe add a comment explaining the purpose of the test.
> 
> And would it be possible to have a test where -vctoolsdir is set to something, to see that it's picked up correctly?
What's the expectation on the test boxes?  I can author such a test but it would be very brittle unless we have a spec for how VS should be installed.


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

https://reviews.llvm.org/D85998