Drop support for Windows SDK versions <8.1

RESOLVED FIXED in mozilla37

Status

()

Core
Build Config
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

Trunk
mozilla37
x86
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
MSVC2012 ships with version 8.0, which is now our minimum supported compiler. We should update the Windows SDK requirements accordingly.
(Assignee)

Comment 1

3 years ago
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

Comment 2

3 years ago
I'm not very familiar with how this should be done, it's best for someone else to take this.
(Assignee)

Comment 3

3 years ago
Looks like jimm was the last person to play around with this.
http://hg.mozilla.org/mozilla-central/file/912036eeb024/configure.in#l415
(Assignee)

Comment 4

3 years ago
I'm going to take a stab at this.
Assignee: nobody → ryanvm
(Assignee)

Comment 5

3 years ago
Created attachment 8544879 [details] [diff] [review]
Set SDK version 603 as the minimum supported

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)
(Assignee)

Comment 7

3 years ago
(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?
(Assignee)

Comment 9

3 years ago
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).
(Assignee)

Comment 10

3 years ago
Created attachment 8545017 [details] [diff] [review]
GFX cleanups

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)
(Assignee)

Comment 11

3 years ago
Created attachment 8545018 [details] [diff] [review]
build system cleanups

winsdk.m4 is much simpler now :)

I've confirmed that these build locally and on Try.
Attachment #8545018 - Flags: review?(mh+mozilla)
(Assignee)

Comment 12

3 years ago
Created attachment 8545020 [details] [diff] [review]
Set SDK version 603 as the minimum supported

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+

Updated

3 years ago
Attachment #8545020 - Flags: feedback?(jacek) → feedback+

Comment 15

3 years ago
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.
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/98162547b084
https://hg.mozilla.org/integration/mozilla-inbound/rev/84708ff7c9fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/6399ee520bcc

I did a quick scan of https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites and didn't see anything obvious that needs updating with this change. Arguably, https://developer.mozilla.org/En/Windows_SDK_versions could use some updating, but it should probably wait until all supported releases are on MSVC2013+ (i.e. when esr31 is EOL).
https://hg.mozilla.org/mozilla-central/rev/98162547b084
https://hg.mozilla.org/mozilla-central/rev/84708ff7c9fe
https://hg.mozilla.org/mozilla-central/rev/6399ee520bcc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Created attachment 8546118 [details] [diff] [review]
Bug 1114577 - Drop support for Windows SDK versions <8.1 - l10n-mozconfig.patch

I think we need to set ac_add_options --with-windows-version=603 in win32/l10n-mozconfig too
Attachment #8546118 - Flags: review?(mshal)

Updated

3 years ago
Attachment #8546118 - Flags: review?(mshal) → review+

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8546488 [details] [diff] [review]
Bug 1114577 - Drop support for Windows SDK versions <8.1 - l10n-mozconfig.patch

re-generated the l10n-mozconfig patch (r+ by mshal) with the right headers.
Attachment #8546118 - Attachment is obsolete: true
Attachment #8546488 - Flags: review+
Keywords: checkin-needed
(Assignee)

Comment 20

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6efc910604a2
Keywords: checkin-needed
(Assignee)

Comment 21

3 years ago
https://hg.mozilla.org/mozilla-central/rev/6efc910604a2
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.