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

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.


================
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.
----------------
zahen wrote:
> 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)
> ```
I realize this is not because of your patch, but it would be nice if this error could be more clear. Ideally it would at least print the path to what's failing to execute. Would you be willing to add that?


================
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
+
----------------
zahen wrote:
> 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.
I'd suggest passing -vctoolsdir "/foo" and check that some /foo dir is getting passed as a system include dir to cc1, and that the link.exe invocation is under /foo.

I don't think it would be brittle.


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

https://reviews.llvm.org/D85998