Drop support for Windows SDK versions <8.1

RESOLVED FIXED in mozilla37

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

Trunk
mozilla37
x86
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

MSVC2012 ships with version 8.0, which is now our minimum supported compiler. We should update the Windows SDK requirements accordingly.
Once bug 1117820 lands, 8.1 is the only release we'll need to support (it comes with MSVC2013). It would be good if we could get this done prior to the next uplift as well.
No longer blocks: 1084532
Depends on: 1084532, 1117820
Summary: Drop support for Windows SDK versions <8.0 → Drop support for Windows SDK versions <8.1
I'm not very familiar with how this should be done, it's best for someone else to take this.
Looks like jimm was the last person to play around with this.
http://hg.mozilla.org/mozilla-central/file/912036eeb024/configure.in#l415
I'm going to take a stab at this.
Assignee: nobody → ryanvm
Looks like this works. With my usual MSVC2012 + WinSDK 8.1 environment, the build succeeds. When I force my local environment to useSDK8 instead of 8.1, configure errors out as expected.

https://tbpl.mozilla.org/?tree=Try&rev=582fabc8fb21 for sanity.

And I intend to create a follow-up patch for other miscellaneous clean-ups.
Attachment #8544879 - Flags: review?(mh+mozilla)
Comment on attachment 8544879 [details] [diff] [review]
Set SDK version 603 as the minimum supported

Review of attachment 8544879 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +437,4 @@
>  MOZ_ARG_WITH_STRING(windows-version,
>  [  --with-windows-version=WINSDK_TARGETVER
> +                          Windows SDK version to target. Win8.1 (603) is
> +                          the only version currently allowed.],

Please rephrase such that we don't need to update (or risk to forget to update) this sentence when a new version appears.

@@ +693,5 @@
>          # strsafe.h on mingw uses macros for function deprecation that pollutes namespace
>          # causing problems with local implementations with the same name.
>          AC_DEFINE(STRSAFE_NO_DEPRECATE)
>  
> +        MOZ_WINSDK_MAXVER=0x06030000

I'm not sure the tests that use this variable will be happy on mingw builds with that value. Jacek?

Note there are many places in configure that test this value, and if it is never going to be below 0x06030000, there are branches of those tests that can be removed as well.
Attachment #8544879 - Flags: review?(mh+mozilla) → feedback?(jacek)
(In reply to Mike Hommey [:glandium] from comment #6)
> I'm not sure the tests that use this variable will be happy on mingw builds
> with that value. Jacek?

In that case, wouldn't a mingw build die during the version check earlier in configure?
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > I'm not sure the tests that use this variable will be happy on mingw builds
> > with that value. Jacek?
> 
> In that case, wouldn't a mingw build die during the version check earlier in
> configure?

which one?
Ah, I see what you mean now. Yeah, I can see the logic in dropping those changes then (though I do wonder if js/src/configure.in should at least be in sync).
Posted patch GFX cleanupsSplinter Review
Mike, can you please take a look at this from a build system standpoint for sanity? And Jeff, can you make sure that gfx changes look OK?

One interesting thing to note: AFAICT, the check in gfxDWriteFontList.cpp appears to have been broken before, so this may in fact change compilation slightly.
Attachment #8545017 - Flags: review?(mh+mozilla)
Attachment #8545017 - Flags: review?(jmuizelaar)
winsdk.m4 is much simpler now :)

I've confirmed that these build locally and on Try.
Attachment #8545018 - Flags: review?(mh+mozilla)
Mike mentioned on IRC that in the long run, we may want to move the mingw max version setting into winsdk.m4 as well. That seems like a sensible idea, but more than I want to do right now. But Jacek, I'm happy to remove those two hunks from the first patch if you want (or change the version in js/src to at least match the other).
Attachment #8544879 - Attachment is obsolete: true
Attachment #8544879 - Flags: feedback?(jacek)
Attachment #8545020 - Flags: review?(mh+mozilla)
Attachment #8545020 - Flags: feedback?(jacek)
Comment on attachment 8545017 [details] [diff] [review]
GFX cleanups

Review of attachment 8545017 [details] [diff] [review]:
-----------------------------------------------------------------

gfxDwriteFontList.cpp looks fine.
Attachment #8545017 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8545017 [details] [diff] [review]
GFX cleanups

Review of attachment 8545017 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/moz.build
@@ +63,3 @@
>          'SourceSurfaceD2DTarget.cpp',
>      ]
> +    DEFINES['USE_D2D1_1'] = True

Might as well remove the define and remove the ifdefs in the code.
Attachment #8545017 - Flags: review?(mh+mozilla) → review+
Attachment #8545018 - Flags: review?(mh+mozilla) → review+
Attachment #8545020 - Flags: review?(mh+mozilla) → review+
Attachment #8545020 - Flags: feedback?(jacek) → feedback+
Thanks for the notice. I tested those patches. A small change for a missing enum values is needed in mingw-w64 itself, but I will take care of that.
I think we need to set ac_add_options --with-windows-version=603 in win32/l10n-mozconfig too
Attachment #8546118 - Flags: review?(mshal)
Attachment #8546118 - Flags: review?(mshal) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
re-generated the l10n-mozconfig patch (r+ by mshal) with the right headers.
Attachment #8546118 - Attachment is obsolete: true
Attachment #8546488 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6efc910604a2
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.