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

[Zachary Henkel via Phabricator via cfe-commits]

zahen 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.
----------------
hans wrote:
> zahen wrote:
> > aganea 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.
> > > @hans The flag proposed here behaves like `env VCToolsInstallDir="" clang-cl ...`, and in that case there's no validation. Do you think the path to `-vctoolsdir` should be validated and throw an error otherwise? I find it nice to completly disable the toolchain auto-detection if passing in a empty dir, without needing an additional flag, ie `clang-cl -vctoolsdir ""`.
> > ```
> > 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?
It's printed when using -v as directed. I don't feel strongly against adding the command line in all failure cases, but did want to point that out.  Still think it's worth it?


================
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:
> 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.
Clever idea! I'll do it that way and key off the "ignored directory" warning that's emitted.


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

https://reviews.llvm.org/D85998