These are still red with VS2017. We kept the clang-cl builders on VS2015 in order to get bug 1408789 landed sooner.

In a proper VS install, the path to cl.exe looks like: ...\VC\Tools\MSVC\14.11.25503\bin\HostX64\x64\cl.exe In our automation, the path is just: ...\VC\bin\HostX64\x64\cl.exe Clang tries to do some sanity-checking to make sure that the cl.exe it finds is the Microsoft compiler and not some other program. But the checks are a little too strict for us, so just look for "bin\Host*\*\cl.exe".

32-bit clang-cl.exe was looking specifically for HostX86\x86\link.exe, which doesn't exist in our automation package. Make it look in HostX64\x86 instead.

Comment on attachment 8924567 [details] [diff] [review] We want a HostX64 linker even with 32-bit clang-cl.exe Review of attachment 8924567 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/build-clang/msvc-host-x64.patch @@ +6,5 @@ > + case SubDirectoryType::Bin: > + if (VSLayout == ToolsetLayout::VS2017OrNewer) { > +- const bool HostIsX64 = > +- llvm::Triple(llvm::sys::getProcessTriple()).isArch64Bit(); > ++ const bool HostIsX64 = true; Why doesn't the original code work in the first place? That seems like a pretty serious bug.

Comment on attachment 8924566 [details] [diff] [review] Loosen clang-cl's MSVC detection to accept our automation's fake paths Review of attachment 8924566 [details] [diff] [review]: ----------------------------------------------------------------- This seems pretty unfortunate, but such is life. ::: build/build-clang/loosen-msvc-detection.patch @@ +7,5 @@ > + // the path. > + // Note: empty strings match anything. > +- llvm::StringRef ExpectedPrefixes[] = {"", "Host", "bin", "", > +- "MSVC", "Tools", "VC"}; > ++ llvm::StringRef ExpectedPrefixes[] = {"", "Host", "bin"}; It might be worth a comment here to indicate the expected vs. actual paths we're matching, and why the original code doesn't work for us?

ni? for comment 4.

(In reply to Nathan Froyd [:froydnj] from comment #4) > Why doesn't the original code work in the first place? That seems like a > pretty serious bug. It works with a proper VS install that has the full matrix of (host)x(target) linkers installed. But our automation package contains only the 64-bit tools.

(In reply to David Major [:dmajor] from comment #7) > (In reply to Nathan Froyd [:froydnj] from comment #4) > > Why doesn't the original code work in the first place? That seems like a > > pretty serious bug. > > It works with a proper VS install that has the full matrix of > (host)x(target) linkers installed. But our automation package contains only > the 64-bit tools. Right, but all we're doing here is: 1) Ask for what we think is the host we're running on; 2) Turn that into an llvm::Triple; 3) Ask if that's 64-bit or not. Which part of that is buggy? It doesn't seem like any of those steps should depend on which host tools we have installed.

(In reply to Nathan Froyd [:froydnj] from comment #8) > 1) Ask for what we think is the host we're running on; It's not looking at the true host, it's checking the current process type, and this compiler is a 32-bit process. To be honest that bothers me a bit, I think a better approach (for which I am not signing up :)) would be to use a 64-bit clang-cl.exe for everything, and pass -m32 as necessary.

I had been leaning towards leaving these in just in case we need to revert compilers, but hey, the beauty of source history is we can always bring them back.

Comment on attachment 8924702 [details] [diff] [review] Build clang-cl itself with VS2017 Review of attachment 8924702 [details] [diff] [review]: ----------------------------------------------------------------- r=me with moving the test_toolchain_configure.py changes to the correct commit. ::: python/mozbuild/mozbuild/test/configure/test_toolchain_configure.py @@ +251,5 @@ > '*.cpp': { > '__STDC_VERSION__': False, > '__cplusplus': '201103L', > }, > + '-fms-compatibility-version=19.11.25547': VS('19.11.25547')[None], All of these test changes should really go in whatever commit changed toolchain.configure, right?

Comment on attachment 8924974 [details] [diff] [review] Remove the now-unused VS2015 manifests Review of attachment 8924974 [details] [diff] [review]: ----------------------------------------------------------------- I feel like it might be better to leave them in for a release cycle (6-8 weeks) so somebody doesn't have to trawl through `hg log` to figure out what the proper values are supposed to be in case we do find ourselves needing to change, but I will leave that decision up to you. Maybe at least leave them in until mozilla-central is officially 59, so they are at least present on beta in case we find ourselves having to scramble there?

(In reply to David Major [:dmajor] from comment #9) > (In reply to Nathan Froyd [:froydnj] from comment #8) > > 1) Ask for what we think is the host we're running on; > > It's not looking at the true host, it's checking the current process type, > and this compiler is a 32-bit process. Aha, that was the context I was missing. Maybe we should summarize this in a comment in the patch file itself? > To be honest that bothers me a bit, I think a better approach (for which I > am not signing up :)) would be to use a 64-bit clang-cl.exe for everything, > and pass -m32 as necessary. Yes, this would be a better solution, and should not really be that hard; we already have code in toolchain.configure to add -m32 where necessary. We just don't have a case for clang-cl. Would you please file a followup bug to a) add that support and b) remove this patch?

Comment on attachment 8924567 [details] [diff] [review] We want a HostX64 linker even with 32-bit clang-cl.exe Review of attachment 8924567 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but let's get that followup bug in.

(In reply to Nathan Froyd [:froydnj] from comment #13) > Comment on attachment 8924974 [details] [diff] [review] > Remove the now-unused VS2015 manifests > > Review of attachment 8924974 [details] [diff] [review]: > ----------------------------------------------------------------- > > I feel like it might be better to leave them in for a release cycle (6-8 > weeks) so somebody doesn't have to trawl through `hg log` to figure out what > the proper values are supposed to be in case we do find ourselves needing to > change, but I will leave that decision up to you. Maybe at least leave them > in until mozilla-central is officially 59, so they are at least present on > beta in case we find ourselves having to scramble there? Leaving them in for 58 beta makes sense. But I probably won't remember to revisit this bug, so I'll just leave them in altogether.

(In reply to Nathan Froyd [:froydnj] from comment #12) > All of these test changes should really go in whatever commit changed > toolchain.configure, right? Hmm, probably. I'll move it.

Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2b705b77cb Loosen clang's MSVC detection to accept our automation's fake paths. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb00b457b92 We want a HostX64 linker even with 32-bit clang-cl.exe. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb92693eac8 Use VS2017 with clang-cl builds. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/dbcddce04a80 Build clang-cl itself with VS2017. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/fb2b705b77cb https://hg.mozilla.org/mozilla-central/rev/ffb00b457b92 https://hg.mozilla.org/mozilla-central/rev/bcb92693eac8 https://hg.mozilla.org/mozilla-central/rev/dbcddce04a80

== Change summary for alert #10448 (as of Thu, 09 Nov 2017 08:02:53 GMT) == Improvements: 2% build times summary windows2012-32 debug static-analysis taskcluster-c4.4xlarge 1,886.39 -> 1,844.34 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10448