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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey Bas - am I somehow configured in a weird way, or have we just accidentally broken building on Windows 7?

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I see the same on a Windows 10 machine with a pretty much vanilla config -- no special settings here.

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Robert Longson [:longsonr]]

Hav you got the windows SDK 8.1 installed? https://msdn.microsoft.com/en-us/library/windows/desktop/dn424972(v=vs.85).aspx

Comment 5

[Jorg K (CEST = GMT+2)]

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

Comment 6

[Frank-Rainer Grahl (:frg)]

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

Comment 7

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 8

[Frank-Rainer Grahl (:frg)]

>> 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

Comment 9

[Bill Gianopoulos [:WG9s]]

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.

Comment 10

[Bill Gianopoulos [:WG9s]]

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.

Comment 11

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

For reference.

Comment 12

[Frank-Rainer Grahl (:frg)]

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.

Comment 13

[Bill Gianopoulos [:WG9s]]

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?

Comment 14

[Bill Gianopoulos [:WG9s]]

(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.

Comment 15

[Bill Gianopoulos [:WG9s]]

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/

Comment 16

[Bill Gianopoulos [:WG9s]]

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.

Comment 17

[Bill Gianopoulos [:WG9s]]

The code doing something completely different than what the comments say, in my opinion would completely invalidate the original r+

Comment 18

[Bas Schouten (:bas.schouten)]

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.

Comment 19

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1352547: Do not use IsWindows10OrGreater() on older SDKs.

Review commit: https://reviewboard.mozilla.org/r/125778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125778/

Comment 20

[Bas Schouten (:bas.schouten)]

This patch should fix things with older SDKs, I've also amended the comment to state why we check for Windows 10.

Comment 21

[Matt Woodrow (:mattwoodrow)]

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?

Comment 22

[Frank-Rainer Grahl (:frg)]

// 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.

Comment 23

[Bill Gianopoulos [:WG9s]]

(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.

Comment 24

[Bill Gianopoulos [:WG9s]]

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)

Comment 25

[Bill Gianopoulos [:WG9s]]

(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.

Comment 26

[Bill Gianopoulos [:WG9s]]

Or is the comment supposed to say:

// We chose not to run this on < Windows 10 because it appears sometimes this breaks

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d186078542
Do not use IsWindows10OrGreater() on older SDKs. r=mattwoodrow

Comment 28

[Bill Gianopoulos [:WG9s]]

(In reply to Pulsebot from comment #27)
> Pushed by [e-mail]:
> 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

[Perry Wagle]

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

[Perry Wagle]

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

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f2d186078542