1223530 - Move MOZ_WINCONSOLE to configure

Status: RESOLVED DUPLICATE of bug 1972782

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

MOZ_WINCONSOLE has been cargo-culted into a bunch of Makefile.ins, but we really could just set it globally in configure.

https://dxr.mozilla.org/mozilla-central/search?q=MOZ_WINCONSOLE+path%3AMakefile.in&redirect=true&case=true

Comment 1

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b4c943e6d3f

Comment 2

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89ce9655e1fa

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 1223530 - Move MOZ_WINCONSOLE to configure. r?gps

bug 1223530 - Move MOZ_WINCONSOLE to configure. r?gps

This patch moves the logic for selecting MOZ_WINCONSOLE out of individual
Makefile.in files and into configure. It also changes config.mk to only
pass -SUBSYSTEM:CONSOLE if MOZ_WINCONSOLE=1. The MSDN docs state that
in the absence of -SUBSYSTEM, the linker will select the proper subsystem
based on whether the program contains [w]main or [w]WinMain, so let it
do that.

One program (windbgdlg) needed a tweak to add a wmain for when MOZ_WINCONSOLE
is defined.

This patch leaves one instance in security/sandbox/win/wow_helper/Makefile.in,
that Makefile has its own separate bug.

Comment 4

[(not currently active) Ted Mielczarek]

Turns out to have been slightly more involved than I expected, but not too bad. This builds fine locally on Windows opt/debug, Try push just to make sure.

Comment 5

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/25261/#review22779

::: toolkit/crashreporter/client/Makefile.in
(Diff revision 1)
> -MOZ_WINCONSOLE = 0

Your change makes the crashreporter client use the console on debug builds. This may cause problems depending whether this was really intended in the first place.

Same for windbgdlg.

::: toolkit/mozapps/update/tests/Makefile.in
(Diff revision 1)
> -MOZ_WINCONSOLE = 1

Here, the console was always enabled. In likeliness, this was done on purpose to debug local opt builds.

::: toolkit/xre/test/win/Makefile.in
(Diff revision 1)
> -MOZ_WINCONSOLE = 1

Same here

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #5)
> https://reviewboard.mozilla.org/r/25261/#review22779
> 
> ::: toolkit/crashreporter/client/Makefile.in
> (Diff revision 1)
> > -MOZ_WINCONSOLE = 0
> 
> Your change makes the crashreporter client use the console on debug builds.
> This may cause problems depending whether this was really intended in the
> first place.

Yes, I know. I tested this locally and it still works fine.

(In reply to Mike Hommey [:glandium] from comment #5)
> Here, the console was always enabled. In likeliness, this was done on
> purpose to debug local opt builds.

This will continue to work fine, since in the absence of MOZ_WINCONSOLE=1, we don't pass any -SUBSYSTEM flag to the linker, so it will choose the proper subsystem based on the type of main symbol present.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8687917 [details]
MozReview Request: bug 1223530 - Move MOZ_WINCONSOLE to configure. r?gps

Reassigning to glandium since he already started review.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8687917 [details]
MozReview Request: bug 1223530 - Move MOZ_WINCONSOLE to configure. r?gps

https://reviewboard.mozilla.org/r/25261/#review23147

Comment 9

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b192fb51bcc

Comment 10

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9181a29e1f2

Comment 11

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b4fa4c8ae0

Comment 12

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0104f1c911ca36aaf413df211ebc7320f61ed853
bug 1223530 - Move MOZ_WINCONSOLE to configure. r=glandium

Comment 13

[Mike Hommey [:glandium]]

This had to be backed out because it broke Windows XP. As philor puts it, "chalk up another victory for not-by-default on try".
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b37512f3f3

This stems from the change in config.mk, because we *do* need to pass -SUBSYSTEM:something because we *also* are specifying a different subsystem version to make things work on Windows XP, which the linker defaults happily don't care about. Which also means we can't rely on the linker picking up the right thing from the main() function used.

Comment 14

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Is this still going to be done? If not I'd like to close the dependent bug.

Comment 15

[(not currently active) Ted Mielczarek]

Yes, I still want to fix this. This might actually be easier in light of the fact that we've dropped WinXP support now.

Comment 16

[Wayne Mery (:wsmwk)]

Is this wanted in time for for version 60?

Comment 17

[Mathew Hodson]

MOZ_WINCONSOLE was removed fro all the makefiles in bug 1972782