Closed
Bug 1001332
Opened 11 years ago
Closed 10 years ago
Add Windows version into /SUBSYSTEM
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla34
People
(Reporter: m_kato, Assigned: away)
References
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
When building VS2012+, subsystem version becomes 6.0, not 5.01 by linker.
We should set subsystem version keeps 5.01 even if VS2012 or VS2013.
Reporter | ||
Comment 1•11 years ago
|
||
Also, _USING_V110_SDK71_ is for XP
Reporter | ||
Comment 2•11 years ago
|
||
_USING_V110_SDK71_ may be unnecessary. If necessary, I will file a new bug.
Summary: Add Windows version into /SUBSYSTEM and define _USING_V110_SDK71_ → Add Windows version into /SUBSYSTEM
Reporter | ||
Comment 3•11 years ago
|
||
Reporter | ||
Comment 4•11 years ago
|
||
Reporter | ||
Comment 5•11 years ago
|
||
Assignee: nobody → m_kato
Attachment #8415736 -
Attachment is obsolete: true
Reporter | ||
Comment 6•11 years ago
|
||
Attachment #8413582 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Attachment #8415757 -
Flags: review?(mh+mozilla)
Reporter | ||
Updated•11 years ago
|
Attachment #8415758 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 7•11 years ago
|
||
VS2012 and VS2013's default is subsystem 6.0. To run on XP, subsystem version should be 5.01.
Comment 8•11 years ago
|
||
Comment on attachment 8415757 [details] [diff] [review]
Part 1. Set subsystem version to 5.01
Review of attachment 8415757 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with the following fixed.
::: config/config.mk
@@ +655,3 @@
> else
> +# For setting subsystem version
> +WIN32_EXE_LDFLAGS += $(WIN32_CONSOLE_EXE_LDFLAGS)
This branch was not there originally. So this means this is trying to replace the default. The default subsystem is windows according to http://msdn.microsoft.com/en-us/library/fcc1zstk%28v=vs.100%29.aspx, so that would be GUI, not CONSOLE.
Attachment #8415757 -
Flags: review?(mh+mozilla) → review+
Updated•11 years ago
|
Attachment #8415758 -
Flags: review?(mh+mozilla) → review+
Comment 9•11 years ago
|
||
According to <http://msdn.microsoft.com/en-us/library/fcc1zstk.aspx>, the lowest possible value is 5.02 for x64.
Assignee | ||
Comment 10•10 years ago
|
||
Can we get this ready for checkin please?
Assignee | ||
Comment 11•10 years ago
|
||
I'll help land this since it's blocking VS2013 rollout. Will address comment 8 and comment 9 and do a sanity check on the build.
Assignee: m_kato → dmajor
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #8)
> This branch was not there originally. So this means this is trying to
> replace the default. The default subsystem is windows according to
> http://msdn.microsoft.com/en-us/library/fcc1zstk%28v=vs.100%29.aspx, so that
> would be GUI, not CONSOLE.
My reading of the documentation is that the default depends on whether you export main or WinMain. The current patch matches the expectations of all our executables, and builds successfully.
If I try to invert the default, I get failures:
error LNK2019: unresolved external symbol _WinMain@16 referenced in function ___tmainCRTStartup
in host_object_int_extract.exe, host_jskwgen.exe, and host_ListCSSProperties.exe.
Assignee | ||
Comment 13•10 years ago
|
||
The current patches don't cover all of our binaries (e.g. freebl3 from NSS), but it seems the loader doesn't care about DLLs, only .exe files. So I propose that we drop the ICU patch and just go with this.
See comment 12 for why I kept the default as console in config.mk.
Attachment #8415757 -
Attachment is obsolete: true
Attachment #8415758 -
Attachment is obsolete: true
Attachment #8479607 -
Flags: review?(mh+mozilla)
Updated•10 years ago
|
Attachment #8479607 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Assignee | ||
Comment 16•10 years ago
|
||
These changes weren't enough. We're missing a subsystem on certutil.exe.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•10 years ago
|
||
(In reply to David Major [:dmajor] from comment #16)
> These changes weren't enough. We're missing a subsystem on certutil.exe.
It's a) not built with our build system, but NSS's, b) not shipped (only used on automation). Do we care?
Assignee | ||
Comment 18•10 years ago
|
||
The mochitests do. https://tbpl.mozilla.org/?tree=Try&rev=637499d15eb8
Assignee | ||
Comment 19•10 years ago
|
||
This makes the Mochitests green on WinXP with VS2013: https://tbpl.mozilla.org/?tree=Try&rev=14cf7e66e2c1 (I pushed as 2013 mozconfig as an ancestor but TBPL doesn't show it)
Attachment #8482140 -
Flags: review?(wtc)
Comment 20•10 years ago
|
||
Comment on attachment 8482140 [details] [diff] [review]
Subsystem for NSS utils
Review of attachment 8482140 [details] [diff] [review]:
-----------------------------------------------------------------
r=wtc.
Patch checked in: https://hg.mozilla.org/projects/nss/rev/7ee8791bd370
Attachment #8482140 -
Flags: review?(wtc)
Attachment #8482140 -
Flags: review+
Attachment #8482140 -
Flags: checkin+
Comment 21•10 years ago
|
||
Comment on attachment 8482140 [details] [diff] [review]
Subsystem for NSS utils
Review of attachment 8482140 [details] [diff] [review]:
-----------------------------------------------------------------
Patch (as part of NSS_3_17_1_BETA2) pushed to mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/e1c5c185b707
Assignee | ||
Comment 22•10 years ago
|
||
Thanks Wan-Teh! The XP tests are working with latest m-c.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 23•10 years ago
|
||
I think NSS change is not enough, because the change doesn't affect freebl3.dll, nssdbm3.dll, softokn3.dll, which use WIN32.mk's OS_DLLFLAGS
Comment 24•10 years ago
|
||
For icuin52.dll, icuuc52.dll, icudt52.dll, the patches are not enough, too.
They use intl/icu/source/common/Makefile.in, intl/icu/source/i18n/Makefile.in 's LDFLAGS, and intl/icu/source/tools/pkgdata/pkgdata.cpp's LINK_FLAGS
Assignee | ||
Comment 25•10 years ago
|
||
It's not necessary to fix the .dll files. The loader only looks at the flags on .exe files. (If this is ever not true, we're screwed, because msvcr120.dll is subsystem 6.00)
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•