Closed Bug 975733 Opened 6 years ago Closed 6 years ago

Move some LDFLAGS for building executables on Windows to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → ehsan
Attachment #8380198 - Flags: review?(mshal)
Attachment #8380198 - Flags: review?(mh+mozilla)
Attachment #8380198 - Flags: review?(gps)
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?
(This is a try push with all of these changes: https://tbpl.mozilla.org/?tree=Try&rev=43da3dfb801a)
(Note that the flag appears in the backend.mk as expected.)
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 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+
Blocked on bug 973649.
Assignee: ehsan → nobody
Depends on: 973649
I honestly wish this stuff just lived in config.mk (or some Python equivalent).
(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.
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.)
(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!  :-)
Please someone file a followup.
(In reply to Mike Hommey [:glandium] from comment #12)
> Please someone file a followup.

Bug 977439
https://hg.mozilla.org/mozilla-central/rev/0c188671f475
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.