[PATCH] D42762: Rewrite the VS Integration Scripts

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/CMakeLists.txt:234
-  set(CPACK_NSIS_EXTRA_UNINSTALL_COMMANDS
-    "ExecWait '$INSTDIR/tools/msbuild/uninstall.bat'")
   if( CMAKE_CL_64 )
----------------
hans wrote:
> Now that the LLVM toolchain installer no longer includes the VS integration, is there some way we could point the user to the marketplace to install the integration, or could we include the vsix in the installer and launch it for them?
Do we have a Release Notes file that we install?  That could be a good place to include a link to the marketplace?

Although, we'd probably want to wait until it's actually published on the marketplace before we do anything.


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:47
+    <CLToolExe>clang-cl.exe</CLToolExe>
+    <CLToolPath>$(LLVMInstallDir)bin</CLToolPath>
+
----------------
hans wrote:
> Is there a way for the user to point to another LLVM installation? This was one of the reasons for separating the toolset from the toolchain, right?
At the moment, you can do this by changing the registry key.  In subsequent patches I can add a UI setting under C/C++ > General.


================
Comment at: llvm/tools/msbuild/clang-cl.xml:1
+<?xml version="1.0" encoding="utf-8"?>
+<Rule Name="clang-cl" PageTemplate="tool" DisplayName="C/C++" SwitchPrefix="/" Order="10" xmlns="http://schemas.microsoft.com/build/2009/properties" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:sys="clr-namespace:System;assembly=mscorlib">
----------------
hans wrote:
> I thought you were going to leave the flag logic out of this patch?
I thought Nico said to only separate out the option remapping into another patch.  I took all of that out.


================
Comment at: llvm/tools/msbuild/license.txt:1
+====================
+LLVM Release License
----------------
hans wrote:
> Can't we use the old file? Checking in another copy seems unfortunate.
I copied this one from the clang-format vs integration folder.  The one at the top level llvm directory has some extra stuff at the bottom which doesn't apply to this, but I don't know if it would still work just as well even with all that extra stuff in it.


================
Comment at: llvm/tools/msbuild/llvm.sln:1
+
+Microsoft Visual Studio Solution File, Format Version 12.00
----------------
hans wrote:
> Should we have a cmake build target to build this vsix, similar to what we do for the clang-format one, which gets built by the "clang_format_vsix" target?
Yea that seems reasonable.  I'll look into how that works.


https://reviews.llvm.org/D42762