Open
Bug 1223530
Opened 9 years ago
Updated 2 years ago
Move MOZ_WINCONSOLE to configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
NEW
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
Reporter | ||
Comment 1•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b4c943e6d3f
Reporter | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89ce9655e1fa
Reporter | ||
Comment 3•9 years ago
|
||
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)
Reporter | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
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
Reporter | ||
Comment 6•9 years ago
|
||
(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•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8687917 -
Flags: review?(mh+mozilla) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8687917 [details] MozReview Request: bug 1223530 - Move MOZ_WINCONSOLE to configure. r?gps https://reviewboard.mozilla.org/r/25261/#review23147
Reporter | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b192fb51bcc
Reporter | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9181a29e1f2
Reporter | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b4fa4c8ae0
Reporter | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0104f1c911ca36aaf413df211ebc7320f61ed853 bug 1223530 - Move MOZ_WINCONSOLE to configure. r=glandium
Comment 13•8 years ago
|
||
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•7 years ago
|
||
Is this still going to be done? If not I'd like to close the dependent bug.
Flags: needinfo?(ted)
Reporter | ||
Comment 15•7 years ago
|
||
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)
Updated•6 years ago
|
Product: Core → Firefox Build System
Comment 16•6 years ago
|
||
Is this wanted in time for for version 60?
Reporter | ||
Updated•5 years ago
|
Assignee: ted → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•