This patch is originally based on D28365, but the clang ToolChain code has been significantly refactored since then, so rebasing that patch on top of tip was difficult / impossible. In any case, I got all of the changes integrated over here, with one notable difference: The header file containing all of the GUIDs and interface definitions is checked directly into the repository. This way it "just works", and we don't have to worry about if the required header file is present, and more importantly, copies of clang built with VS 2015 can run on a machine with VS 2017 installed and still properly detect the VS 2017 install. This would not be possible otherwise. I've added dberlin as a reviewer for the portion of this CL that involves checking in the header file.
Details
Diff Detail
Event Timeline
clang/lib/Driver/ToolChains/MSVCSetupApi.h | ||
---|---|---|
3 | There is no project root, you should paste the LICENSE.txt into the header directly. |
clang/lib/Driver/ToolChains/MSVCSetupApi.h | ||
---|---|---|
3 | Is a third_party directory directly in this folder ok, or should it be some kind of top level third_party directory? |
clang/lib/Driver/ToolChains/MSVCSetupApi.h | ||
---|---|---|
3 | Normal practice for our open source projects (ie not llvm) would be top-level, because otherwise they tend to be hard to notice. |
clang/lib/Driver/ToolChains/MSVCSetupApi.h | ||
---|---|---|
3 | LLVM, in typical practice, does not segregate small amounts of third party code. For example: |
clang/lib/Driver/ToolChains/MSVCSetupApi.h | ||
---|---|---|
3 | FWIW, this is likely a remarkably bad idea :) |
clang/lib/Driver/ToolChains/MSVCSetupApi.h | ||
---|---|---|
3 | Given that there's already precedent for doing de-segregating (is that the right word here?) small pieces of 3rd party code, is it ok for this to go in as-is (after pasting the LICENSE.txt into the header) and then after the foundation reaches a decision, we come back and address it all at once? |
clang/lib/Driver/ToolChains/MSVCSetupApi.h | ||
---|---|---|
3 | I'm not going to approve it, since i think making the problem worse is also a really bad idea, but if someone else does, and you commit it, i'm not going to take offense. |
clang/lib/Driver/ToolChains/MSVCSetupApi.h | ||
---|---|---|
3 | How about this then? For now, I will assume that if _MSC_VER >= 1910 (i.e. you are building with VS 2017), then you also have the Microsoft header, and I will include the Microsoft header and this codepath will work. Otherwise, you get the old codepath (meaning that if you build clang with MSVC <2017, then you also cannot use the built clang to achieve compatibility with MSVC 2017 even if you copy it to a machine that has MSVC 2017 installed.) Once you hear back and reach a decision, we can get this header checked in and enable clang-cl bootstrapped with MSVC 2015 to be compatible with MSVC >= 2017. |
I don't think any Visual Studio versions ship with the header, it needs to be installed via Nuget
Yea I realized that when I removed the header locally and tried to find the one to include. That's a bummer.
There is no project root, you should paste the LICENSE.txt into the header directly.
Also, generally, i would segregate this into a third_party directory, as keeping code like this directly in the rest of the project makes it a lot messier