Closed
Bug 1352547
Opened 8 years ago
Closed 8 years ago
Can't build on Windows since bug 1352163 landed (error C3861: 'IsWindows10OrGreater': identifier not found)
Categories
(Core :: Graphics: Layers, enhancement)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mconley, Unassigned)
References
Details
(Whiteboard: [Thunderbird-testfailure: B Windows])
Attachments
(1 file, 1 obsolete file)
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.
Reporter | ||
Comment 1•8 years ago
|
||
Hey Bas - am I somehow configured in a weird way, or have we just accidentally broken building on Windows 7?
Flags: needinfo?(bas)
Comment 2•8 years ago
|
||
I see the same on a Windows 10 machine with a pretty much vanilla config -- no special settings here.
Updated•8 years ago
|
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
Updated•8 years ago
|
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)
Comment 3•8 years ago
|
||
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]
Comment 4•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
>> 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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
For reference.
Comment 12•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
The code doing something completely different than what the comments say, in my opinion would completely invalidate the original r+
Comment 18•8 years ago
|
||
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) |
Comment 20•8 years ago
|
||
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 years 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+
Comment 22•8 years ago
|
||
// 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.
Updated•8 years ago
|
Attachment #8853661 -
Attachment is obsolete: true
Comment 23•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
Or is the comment supposed to say:
// We chose not to run this on < Windows 10 because it appears sometimes this breaks
Updated•8 years ago
|
Flags: needinfo?(bas)
Comment 27•8 years ago
|
||
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d186078542
Do not use IsWindows10OrGreater() on older SDKs. r=mattwoodrow
Comment 28•8 years ago
|
||
(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 years ago
|
||
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 years ago
|
||
I was confused. I meant "hg blame" on my windows 10 build machine (running vs2015). DXR doesn't blame anyone.
Comment 31•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
Flags: needinfo?(bas)
You need to log in
before you can comment on or make changes to this bug.
Description
•