Open Bug 1223530 Opened 4 years ago Updated 8 months ago

Move MOZ_WINCONSOLE to configure

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: ted, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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
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.
Attachment #8687917 - Flags: review?(gps)
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.
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
(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 on attachment 8687917 [details]
MozReview Request: bug 1223530 - Move MOZ_WINCONSOLE to configure. r?gps

Reassigning to glandium since he already started review.
Attachment #8687917 - Flags: review?(gps) → review?(mh+mozilla)
Attachment #8687917 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8687917 [details]
MozReview Request: bug 1223530 - Move MOZ_WINCONSOLE to configure. r?gps

https://reviewboard.mozilla.org/r/25261/#review23147
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.
Blocks: 1228450
Is this still going to be done? If not I'd like to close the dependent bug.
Flags: needinfo?(ted)
Yes, I still want to fix this. This might actually be easier in light of the fact that we've dropped WinXP support now.
Flags: needinfo?(ted)
Product: Core → Firefox Build System
Is this wanted in time for for version 60?
Assignee: ted → nobody
You need to log in before you can comment on or make changes to this bug.