Closed
Bug 975733
Opened 11 years ago
Closed 11 years ago
Move some LDFLAGS for building executables on Windows to moz.build
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla30
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ehsan
Blocks: xulinmozbuild
Assignee | ||
Updated•11 years ago
|
Attachment #8380198 -
Flags: review?(mshal)
Attachment #8380198 -
Flags: review?(mh+mozilla)
Attachment #8380198 -
Flags: review?(gps)
Assignee | ||
Comment 2•11 years ago
|
||
Hmm... this patch makes the /HEAP flags disappear from the build for some reason. See for example:
<https://tbpl.mozilla.org/php/getParsedLog.php?id=35103788&tree=Try&full=1>
Any idea what's going on here?
Assignee | ||
Comment 3•11 years ago
|
||
(This is a try push with all of these changes: https://tbpl.mozilla.org/?tree=Try&rev=43da3dfb801a)
Assignee | ||
Comment 4•11 years ago
|
||
(Note that the flag appears in the backend.mk as expected.)
Comment 5•11 years ago
|
||
I just made a similar comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=973649#c13
I think we need to change config/config.mk either in where it includes backend.mk, or how it sets CFLAGS/CXXFLAGS/LDFLAGS, or maybe make backend.mk write to OS_LDFLAGS instead of LDFLAGS
Comment 6•11 years ago
|
||
Comment on attachment 8380198 [details] [diff] [review]
Move some LDFLAGS for building executables on Windows to moz.build
>diff --git a/embedding/tests/winEmbed/moz.build b/embedding/tests/winEmbed/moz.build
>index 6608d50..ab108b0 100644
>--- a/embedding/tests/winEmbed/moz.build
>+++ b/embedding/tests/winEmbed/moz.build
>@@ -12,8 +12,22 @@ SOURCES += [
> 'winEmbed.cpp',
> ]
>
> XPI_NAME = 'winembed'
>
> DEFINES['XPCOM_GLUE'] = True
>
> RESFILE = 'winEmbed.res'
>+
>+# Control the default heap size.
>+# This is the heap returned by GetProcessHeap().
>+# As we use the CRT heap, the default size is too large and wastes VM.
>+#
>+# The default heap size is 1MB on Win32.
>+# The heap will grow if need be.
>+#
>+# Set it to 256k. See bug 127069.
>+if not CONFIG['GNU_CC']:
>+ LDFLAGS += ['/HEAP:0x40000']
>+else:
>+ # Get rid of console window
>+ LDFLAGS += ['-mwindows']
nit: We could flip this to avoid the 'if not...else'. Not a big deal though.
+if CONFIG['GNU_CC']:
+ # Get rid of console window
+ LDFLAGS += ['-mwindows']
+else:
+ # Control the default heap size.
... (long comment)
+ LDFLAGS += ['/HEAP:0x40000']
Attachment #8380198 -
Flags: review?(mshal)
Attachment #8380198 -
Flags: review?(mh+mozilla)
Attachment #8380198 -
Flags: review?(gps)
Attachment #8380198 -
Flags: review+
Comment 8•11 years ago
|
||
I honestly wish this stuff just lived in config.mk (or some Python equivalent).
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to comment #8)
> I honestly wish this stuff just lived in config.mk (or some Python equivalent).
I keep telling people please let's not have perfect be the enemy of the good. The goal of making it possible to build libxul from moz.build will have tangible benefits beyond code elegance, so I would like to kindly request everyone to not let these wishes block landing these patches. In the future we can still move some of this stuff to config.mk and remove them from moz.build. No need to block moving these out of Makefile.in's in the mean time.
Comment 10•11 years ago
|
||
Yeah, sorry, that was just a gripe, not intended as stop energy. (Just seems silly that we have this stuff in every single makefile that builds an exe.)
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to comment #10)
> Yeah, sorry, that was just a gripe, not intended as stop energy. (Just seems
> silly that we have this stuff in every single makefile that builds an exe.)
That I agree with! :-)
Comment 12•11 years ago
|
||
Please someone file a followup.
Assignee | ||
Comment 13•11 years ago
|
||
Assignee: nobody → ehsan
Assignee | ||
Comment 14•11 years ago
|
||
Comment 15•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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
•