1001332 - Add Windows version into /SUBSYSTEM

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Makoto Kato [:m_kato]]

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.

Comment 1

[Makoto Kato [:m_kato]]

Also, _USING_V110_SDK71_ is for XP

Comment 2

[Makoto Kato [:m_kato]]

_USING_V110_SDK71_ may be unnecessary.  If necessary, I will file a new bug.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: for ICU

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Always set SUBSYSTEM version to 5.01

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Part 1. Set subsystem version to 5.01

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Part 2. Set subsystem version for building ICU

Comment 7

[Makoto Kato [:m_kato]]

VS2012 and VS2013's default is subsystem 6.0.  To run on XP, subsystem version should be 5.01.

Comment 8

[Mike Hommey [:glandium]]

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.

Comment 9

[Masatoshi Kimura [:emk]]

According to <http://msdn.microsoft.com/en-us/library/fcc1zstk.aspx>, the lowest possible value is 5.02 for x64.

Comment 10

[(Away)]

Can we get this ready for checkin please?

Comment 11

[(Away)]

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.

Comment 12

[(Away)]

(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.

Comment 13

[(Away)]

Attachment: Set subsystem version 5.01/5.02

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.

Comment 14

[(Away)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/437f9ad3fec9

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/437f9ad3fec9

Comment 16

[(Away)]

These changes weren't enough. We're missing a subsystem on certutil.exe.

Comment 17

[Mike Hommey [:glandium]]

(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?

Comment 18

[(Away)]

The mochitests do. https://tbpl.mozilla.org/?tree=Try&rev=637499d15eb8

Comment 19

[(Away)]

Attachment: Subsystem for NSS utils

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)

Comment 20

[Wan-Teh Chang]

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

Comment 21

[Wan-Teh Chang]

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

Comment 22

[(Away)]

Thanks Wan-Teh! The XP tests are working with latest m-c.

Comment 23

[xunxun]

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

[xunxun]

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

Comment 25

[(Away)]

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)