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

Mon Aug 24 10:30:58 PDT 2020

hans added a comment.

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

> The build system strives to be deterministic so all file probes need to be accounted for; environment variables are also problematic.  Our builds deliberately don't run from a Visual Studio command prompt.  In addition, we'd like to avoid coupling clang tools that don't perform codegen to link.exe.  We do use -fmsc-version= during the build.

How does the build system work with regular cl.exe if environment variables are problematic?

================
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.
----------------
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.

================
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
+
----------------
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?

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

https://reviews.llvm.org/D85998