[PATCH] D39994: Loosen MSVC 2017 path requirements
dmajor added a comment. > Anyway, I'm just venting. If rnk@ wants to lgtm this, I'm fine. @rnk, any objection to this patch? https://reviews.llvm.org/D39994 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39994: Loosen MSVC 2017 path requirements
zturner added a comment. I'm not suuuper opposed, but at the same time if this code is bothering people (and it is, consistently), I don't changing the requirements from "confusing rule A" to "confusing rule B" is going to solve the long term burden that people keep running into. Not asking you to do this work, but my ideal solution is probably to teach clang-cl to recognize 3 new environment variables. `CLANGCL_MSVC_BIN` - Where to look for `cl.exe`, and `link.exe`. Under no circumstances do we consult `PATH` or anything else. This is only used when we need to fallback to `cl` (rarely, anymore) or when the compiler needs to invoke the linker. But! At the same time we make `-fuse-ld=lld` the default. We only do something else if the user specifies `-fuse-ld=link`, and in that case it uses `CLANGCL_MSVC_BIN` (or if you specified `-fuse-ld=` then the env var isn't needed). `CLANGCL_WINSDK` - Points to the root of the Windows SDK. The folder here should have a "standard" layout so that it least pretends to be an installation, so that we can find the right lib directory when cross-compiling. `CLANGCL_MSVCRT` - Same as before, but for the CRT. I would honestly like to delete just about 100% of this stuff about finding MSVC. We should just use exactly what we're told to use and nothing else. Simple, easy to understand, and easy to explain. Anyway, I'm just venting. If rnk@ wants to lgtm this, I'm fine. https://reviews.llvm.org/D39994 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39994: Loosen MSVC 2017 path requirements
hans added a comment. I think the patch is fine, but Zach should probably sign off on it. https://reviews.llvm.org/D39994 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39994: Loosen MSVC 2017 path requirements
rnk added a comment. I haven't dug into this code to really understand if this is right and won't change our version detection logic, but yes, broadly I believe we should just consult PATH, find a cl.exe, and check its version. I'd like to reduce this path validation to a minimum. https://reviews.llvm.org/D39994 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39994: Loosen MSVC 2017 path requirements
dmajor created this revision.
Herald added subscribers: kristof.beyls, aemerson.
Mozilla's build machines are currently applying this patch locally, but I
thought I'd offer it upstream because it should be pretty harmless.
clang-cl has some sanity checks to make sure that the cl.exe it finds is
actually the Microsoft compiler and not something else. The code expects the
path to look like:
...\VC\Tools\MSVC\14.11.25503\bin\HostX64\x64\cl.exe
It doesn't work on Mozilla's build automation where we have a fake Visual
Studio installation that looks like: c:\vs2017\VC\bin\HostX64\x64\cl.exe
This patch reduces the path requirement to `...\bin\Host*\*\cl.exe`.
https://reviews.llvm.org/D39994
Files:
lib/Driver/ToolChains/MSVC.cpp
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -152,8 +152,7 @@
// path components with these prefixes when walking backwards through
// the path.
// Note: empty strings match anything.
-llvm::StringRef ExpectedPrefixes[] = {"", "Host", "bin", "",
- "MSVC", "Tools", "VC"};
+llvm::StringRef ExpectedPrefixes[] = {"", "Host", "bin"};
auto It = llvm::sys::path::rbegin(PathEntry);
auto End = llvm::sys::path::rend(PathEntry);
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -152,8 +152,7 @@
// path components with these prefixes when walking backwards through
// the path.
// Note: empty strings match anything.
-llvm::StringRef ExpectedPrefixes[] = {"", "Host", "bin", "",
- "MSVC", "Tools", "VC"};
+llvm::StringRef ExpectedPrefixes[] = {"", "Host", "bin"};
auto It = llvm::sys::path::rbegin(PathEntry);
auto End = llvm::sys::path::rend(PathEntry);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
