Closed
Bug 1114577
Opened 8 years ago
Closed 8 years ago
Drop support for Windows SDK versions <8.1
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla37
People
(Reporter: RyanVM, Assigned: RyanVM)
References
Details
Attachments
(4 files, 2 obsolete files)
7.05 KB,
patch
|
glandium
:
review+
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
3.61 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
4.45 KB,
patch
|
glandium
:
review+
jacek
:
feedback+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
massimo
:
review+
|
Details | Diff | Splinter Review |
MSVC2012 ships with version 8.0, which is now our minimum supported compiler. We should update the Windows SDK requirements accordingly.
Assignee | ||
Comment 1•8 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.
Comment 2•8 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•8 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 5•8 years ago
|
||
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 6•8 years ago
|
||
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•8 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?
Comment 8•8 years ago
|
||
(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•8 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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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 13•8 years ago
|
||
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 14•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8545018 -
Flags: review?(mh+mozilla) → review+
Updated•8 years ago
|
Attachment #8545020 -
Flags: review?(mh+mozilla) → review+
Updated•8 years ago
|
Attachment #8545020 -
Flags: feedback?(jacek) → feedback+
Comment 15•8 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•8 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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 18•8 years ago
|
||
I think we need to set ac_add_options --with-windows-version=603 in win32/l10n-mozconfig too
Attachment #8546118 -
Flags: review?(mshal)
Updated•8 years ago
|
Attachment #8546118 -
Flags: review?(mshal) → review+
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 19•8 years ago
|
||
re-generated the l10n-mozconfig patch (r+ by mshal) with the right headers.
Attachment #8546118 -
Attachment is obsolete: true
Attachment #8546488 -
Flags: review+
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6efc910604a2
Keywords: checkin-needed
Assignee | ||
Comment 21•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6efc910604a2
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•