Can't build on Windows since bug 1352163 landed (error C3861: 'IsWindows10OrGreater': identifier not found)

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: Layers
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: mconley, Unassigned)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [Thunderbird-testfailure: B Windows])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Bug 1352163 added a call to IsWindows10OrGreater(). That's apparently not supported on my Windows 7 machine, if I'm reading https://msdn.microsoft.com/en-us/library/windows/desktop/dn905474(v=vs.85).aspx correctly.

Perhaps we're trying to take advantage of lazy evaluation here, and I should have even been able to evaluate that part of the condition, but, well, I did.
Hey Bas - am I somehow configured in a weird way, or have we just accidentally broken building on Windows 7?
Flags: needinfo?(bas)
I see the same on a Windows 10 machine with a pretty much vanilla config -- no special settings here.
Summary: Can't build on my Windows 7 machine since bug 1352163 landed → Can't build on Windows since bug 1352163 landed
Version: 50 Branch → Trunk
Summary: Can't build on Windows since bug 1352163 landed → Can't build on Windows since bug 1352163 landed (error C3861: 'IsWindows10OrGreater': identifier not found)
Mike, thanks for reporting, I can't build Thunderbird either, Win 7. Allow me to add our tracking flags to the whiteboard. I'm tracking any issues we see on C-C treeherder, "B Windows" means that the bug is responsible for Build failures on Windows.
Whiteboard: [Thunderbird-testfailure: B Windows]
Hav you got the windows SDK 8.1 installed? https://msdn.microsoft.com/en-us/library/windows/desktop/dn424972(v=vs.85).aspx
I'm not sure whom you're asking. How do I tell whether SDK 8.1 is installed? I have:
C:\Program Files (x86)\Windows Kits\8.0 8.1 and 10. Under "Programs and Features" I see:
Windows Software Development Kit for Windows 8.1, 24/11/2014, 8.100.26898. I doubt that in 2014 an function testing for Windows 10 would have been included. So my environment matches
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites
I have an 8.1 SDK installed from 2013 and it does not contain the funnction in VersionHelpers.h. But the 8.1 SDK was updated in April 2015 according to the website:

https://developer.microsoft.com/en-us/windows/downloads/windows-8-1-sdk

I bet it is included in the latest version. I am using the Windows 10 10586 SDK on Windows 7. You need to set  

SET SDKDIR='C:\Program Files (x86)\Windows Kits\10'

VS2015 probably installed it along the 8.1 SDK so if you have this directory you might only need to change the environment.

Installing the latest 14393 Windows 10 SDK on Windows 7 is broken. This might have changed with the latest build shipping with VS2017 and also now available on the SDK page. I didn't try yet. But 10586 installs fine and you can still get it from here:

https://developer.microsoft.com/en-us/windows/downloads/sdk-archive#more-text
OK, I updated from
https://developer.microsoft.com/en-us/windows/downloads/windows-8-1-sdk
That got me to 8.100.26936. Sadly, no improvement:
error C3861: 'IsWindows10OrGreater': identifier not found

Next I tried:
SET SDKDIR='C:\Program Files (x86)\Windows Kits\10'
call c:\mozilla-build\start-shell-msvc2015-x64.bat
Still error C3861: 'IsWindows10OrGreater': identifier not found.
I didn't do a clobber build though.

So the only thing that helps is: hg backout -r 1765f0af5161 ;-)
>> That got me to 8.100.26936. Sadly, no improvement:

Looks so. I just downloaded it and it has the same content as my old one from 2015. I would try a clobber as a last resort. Not sure if the sdk paths in the caches are updated without at least a configure.

>> So the only thing that helps is: hg backout -r 1765f0af5161 ;-)

Unless the Windows 10 SDK is being made mandatory probably yes.

I have 2 VersionHelpers.h on disc.

This one is used and contains the function:
> c:\Program Files (x86)\Windows Kits\10\Include\10.0.10586.0\um\VersionHelpers.h

From 2013 and not containing it:
> c:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h
Well bug 1352163 patch is odd anyway.  the comment on the include for VersionHelpers specifically says it is so the isWindows8OrGreater will be defined, but then the code actually users isWindows10OrGreater.
To be clear here, I am talking about the include added by the patch here:

+include <VersionHelpers.h> // For IsWindows8OrGreater

and the code added here:

+    if (gfxPrefs::Direct3D11UseDoubleBuffering() && SUCCEEDED(hr) && dxgiFactory2 && IsWindows10OrGreater()) {
+      // DXGI_SCALING_NONE is not available on Windows 7 with Platform Update.
+      // This looks awful for things like the awesome bar and browser window resizing
+      // so we don't use a flip buffer chain here. When using EFFECT_SEQUENTIAL
+      // it looks like windows doesn't stretch the surface when resizing.

Which actually tests for isWindows10Or Greater, despite the fact that the comments say it is to prevent doing this on Windows 7, so testing for isWindows8OrGreater should b3 sufficient.

I am thinking this is a typo.  Unfortunately I have no Windows 8 system to test on to make sure changing the test to isWindows8OrGreater does not cause issues on Windows 8.
Created attachment 8853661 [details] [diff] [review]
Workaround

For reference.
For what's it worth. The latest Windows 10 SDK 10.1.14393.795 installs again on Windows 7. Clobber needed (I blew away the complete object dirs) or it won't work.
But that still leaves my question.  There is a Windows SDK the installs with Visual Studio 2015 community edition.  However, it does not install the registry keys that Mozilla Build looks for.  However the latest version of Mozilla Build, at least a week ago did not look for the latest Windows SDK and instead insisted on finding the Windows 8.1 SDK so exactly why would I expect installing the stand-alone Windows 10 SDK to fix this and if it does, Why is it not listed on the Windows Build requirements page?
(In reply to Bill Gianopoulos [:WG9s] from comment #13)
> But that still leaves my question.  There is a Windows SDK the installs with
> Visual Studio 2015 community edition.  However, it does not install the
> registry keys that Mozilla Build looks for.  However the latest version of
> Mozilla Build, at least a week ago did not look for the latest Windows SDK
> and instead insisted on finding the Windows 8.1 SDK so exactly why would I
> expect installing the stand-alone Windows 10 SDK to fix this and if it does,
> Why is it not listed on the Windows Build requirements page?

BTW this is the second bug I have asked these same questions in.  Waiting 2 days for an answer in the other one.
I have builds available that were done under Windows 10 with visual studio 2015 after changing the isWindows10OrGreater to isWindows8OrGreater.  If people with Windows 8 could install those builds and test to see if they see either of these issues:

// This looks awful for things like the awesome bar and browser window resizing

I think that would be helpful in resolving this bug.

The builds are available at http://www.wg9s.com/mozilla/firefox/
And just to make my position clear this is either a typo, and the if should be testing for isWindows8OrGreater, or the entire patch needs to be backed out because the code is completely inconsistent with the comments.
The code doing something completely different than what the comments say, in my opinion would completely invalidate the original r+
Yeah, this I think is due to standard Mozilla build not finding the SDK supplied with Visual Studio that our build bots use but the 8.1 SDK. Yes, the comment is about something else, this should be IsWindows10OrGreater, I suppose the whole block should then be wrapped in some kind of SDKVER check.
Flags: needinfo?(bas)
Comment hidden (mozreview-request)
This patch should fix things with older SDKs, I've also amended the comment to state why we check for Windows 10.

Comment 21

8 months ago
mozreview-review
Comment on attachment 8853704 [details]
Bug 1352547: Do not use IsWindows10OrGreater() on older SDKs.

https://reviewboard.mozilla.org/r/125778/#review128296

::: gfx/layers/d3d11/CompositorD3D11.cpp:1374
(Diff revision 1)
> -  // ClearRect will set the correct blend state for us.
> -  ClearRect(Rect(mCurrentClip.x, mCurrentClip.y, mCurrentClip.width, mCurrentClip.height));
> +  IntRegion regionToClear(mCurrentClip);
> +  regionToClear.Sub(regionToClear, aOpaqueRegion);
> +
> +  ClearRect(Rect(regionToClear.GetBounds()));
> +
> +  mContext->OMSetBlendState(mAttachments->mPremulBlendState, sBlendFactor, 0xFFFFFFFF);

These chunks already landed with the original patch, right?
Attachment #8853704 - Flags: review?(matt.woodrow) → review+
// We chose not to run this on Windows 10 because it appears sometimes this breaks

Is the comment right. Shouldn't it say Windows 8? Apparently the code now runs under 10.
Attachment #8853661 - Attachment is obsolete: true
(In reply to Bas Schouten (:bas.schouten) from comment #20)
> This patch should fix things with older SDKs, I've also amended the comment
> to state why we check for Windows 10.

The last 3 hunks of this patch seem to be changes that are already present on Mozilla-central.
Basically, everything starting from this point on is extraneous:

@@ -818,16 +824,20 @@ CompositorD3D11::GetPSForEffect(Effect*
     NS_WARNING("No shader to load");
     return nullptr;
   }
 }

 void
 CompositorD3D11::ClearRect(const gfx::Rect& aRect)
(In reply to Frank-Rainer Grahl from comment #22)
> // We chose not to run this on Windows 10 because it appears sometimes this
> breaks
> 
> Is the comment right. Shouldn't it say Windows 8? Apparently the code now
> runs under 10.

If I am reading the code comments correctly then shouldn't the if be testing for !isWIndows10OrGreater?  Basically I do not how the code is doing hat the comments say.
Or is the comment supposed to say:

// We chose not to run this on < Windows 10 because it appears sometimes this breaks
Flags: needinfo?(bas)

Comment 27

8 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d186078542
Do not use IsWindows10OrGreater() on older SDKs. r=mattwoodrow
(In reply to Pulsebot from comment #27)
> Pushed by bschouten@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d186078542
> Do not use IsWindows10OrGreater() on older SDKs. r=mattwoodrow

Most of the time people think I am just a PITA with my comments.  I am glad my comments here seemed to have been helpful and taken into account in what landed.

Comment 29

8 months ago
confused
Why does DXR on the errant line defer "blame" to a 11 year old bug instead of the recent one?

Should I file a bug for that?

Comment 30

8 months ago
I was confused.  I meant "hg blame" on my windows 10 build machine (running vs2015).  DXR doesn't blame anyone.

Comment 31

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f2d186078542
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.