Opened 12 years ago
Closed 10 years ago
#1516 closed defect (fixed)
[PATCH] premake adds random flags
Reported by: | Julian Ospald | Owned by: | |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 11 |
Component: | Build & Packages | Keywords: | |
Cc: | Patch: |
Description
x86_64-pc-linux-gnu-g++ -march=native -O2 -pipe -Wall -ggdb -march=native -O2 -pipe -Wall -ggdb -MMD -MP -DNDEBUG -DCONFIG_FINAL=1 -DCONFIG2_FAM=0 -DLIB_STATIC_LINK -DINSTALLED_BINDIR=/usr/games/bin -DINSTALLED_DATADIR=/usr/share/games/0ad -DINSTALLED_LIBDIR=/usr/games/lib64/0ad -DWITH_SYSTEM_MOZJS185 -I/usr/X11R6/include/X11 -I/usr/X11R6/include -I/usr/include/X11 -I../../../source/pch/gui -I../../../source -g -Wall -O3 -Wno-switch -Wno-reorder -Wno-invalid-offsetof -Wextra -Wno-missing-field-initializers -Wunused-parameter -Wredundant-decls -Wnon-virtual-dtor -Wundef -fstack-protector-all -D_FORTIFY_SOURCE=2 -fstrict-aliasing -fpch-preprocess -fno-omit-frame-pointer -msse -fvisibility=hidden pkg-config mozjs185 --cflags
sdl-config --cflags
-MF obj/gui_Release/CInput.d -MT "obj/gui_Release/CInput.o" -o "obj/gui_Release/CInput.o" -c "../../../source/gui/CInput.cpp"
IGUIObject.cpp
As you can see the build system adds random flags and overwrites some system flags including optimization, hiding warnings and such.
Attachments (5)
Change History (25)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
-march=native -O2 -pipe -Wall -ggdb -march=native -O2 -pipe -Wall -ggdb
are the system flags (funny thing it adds both cflags and cxxflags btw like SCons)
-g -Wall -O3 -Wno-switch -Wno-reorder -Wno-invalid-offsetof -Wextra -Wno-missing-field-initializers -Wunused-parameter -Wredundant-decls -Wnon-virtual-dtor -Wundef -fstack-protector-all -fstrict-aliasing -fpch-preprocess -fno-omit-frame-pointer -msse -fvisibility=hidden
are flags that get added and may overwrite my own flags. I doubt that "-O3" is needed to compile 0ad. Upstream should IMO only add flags that are _crucial_ for compilation. This is not the case for most of them i guess. If you want to provide a sane lot of optimization flags it should be put behind an option. I tried to hack it, but premake is so borked by design I lost my patience. I would rather be willing to help on the cmake project...
comment:4 by , 12 years ago
Those flags are triggered (at least most of them) by our premake4.lua. I guess most of those flags aren't needed, but some sane defaults ease the development.
So would adding a flag to premake that only adds the needed (e.g. -fno-strict-aliasing for collada) flags be ok? (We could even make that the default for some OSs, but I don't think we should differentiate between them too much)
Just that I note it here: #1416 suggests something along those lines too.
by , 12 years ago
Attachment: | premake_flags.diff added |
---|
comment:5 by , 12 years ago
The attached wip patch adds --disable-preset-flags
and --minimal-flags
(I only tested the latter).
Not all flags are removed on all OS, and some (non-default) options still add flags. I also modified the use of -rpath
as noted in #1421.
comment:6 by , 12 years ago
Most people who build 0 A.D. don't want to worry about compiler flags, regardless of their OS, and there's a reason why all of those flags are added (usually explained in premake4.lua
). So I agree with leper's solution, which is to make removing the flags optional. I can see how that would help with packaging and building on certain platforms.
On a related note, we should document all of our Premake options somewhere if they're not already.
by , 12 years ago
Attachment: | premake_flags.2.diff added |
---|
follow-up: 8 comment:7 by , 12 years ago
Removed --disable-preset-flags
(it did add to much clutter) (and it only removed -fno-strict-aliasing
which is needed for Collada). If someone really wants his build failing (just that he can add that back) he can maintain a patch for that.
--minimal-flags
doesn't strip any files now. I also modified the sed line for OS X to strip the binaries there.
-g
is still added, but I wont have much time in the near future, so if someone wants to fix that go on.
Some testing of the --with-system-mozjs185
parameter (regarding #1421) would be nice.
Replying to historic_bruno:
On a related note, we should document all of our Premake options somewhere if they're not already.
Any idea for a good place?
comment:8 by , 12 years ago
Replying to leper:
Replying to historic_bruno:
On a related note, we should document all of our Premake options somewhere if they're not already.
Any idea for a good place?
How about a new page linked from BuildInstructions? (in the steps related to update-workspaces.sh
)
comment:9 by , 12 years ago
Premake is ridiculous. You have to build it twice, so the fixes apply unless you want to edit this mess of scripts.c directly.
The Premake Makefile and Fcollada Makefile also overwrite/add some optimization flags.
comment:10 by , 12 years ago
Apparently -O2 causes issues for FCollada, I will look into that and what gcc versions are still affected. Probably better to introduce a switch then and keep -O1 as default.
by , 12 years ago
Attachment: | optimization-flags.patch added |
---|
fixed compilation for -O2 and -O3 in FCollada, so we don't have to force -O1 anymore
comment:12 by , 12 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 11 |
Priority: | Should Have → Nice to Have |
Summary: | premake adds random flags → [PATCH] premake adds random flags |
Moving this to the Alpha 11 Milestone and adding review because of hasufell's patch. (For my patch some testing is still required and r12069 needs a slight modification.)
comment:13 by , 12 years ago
r12072 should make the fcollada/src/Makefile
CXXFLAGS
change unnecessary - it should build fine with the default -O2
/-O3
behaviour.
by , 12 years ago
Attachment: | 0ad-r11339-trac-1421.patch added |
---|
Rediffed as I understood it, for (late) feedback of #1421 comment, and it works in not adding rpath in a rpm build
follow-up: 16 comment:14 by , 12 years ago
About the premake4.lua
patch: line 338 in project_create()
looks redundant because the flag is already set a few lines earlier in project_set_build_flags()
. I'm not sure what was intended.
To address the comment on line 332, I don't think we should check for minimal-flags before adding to the rpath, since that will most likely result in a broken build if bundled Spidermonkey was used. The issue was mostly that it's unnecessary if --with-system-mozjs185
is used and the patch fixes that :)
IMO minimal-flags should have no effect on Windows builds, mostly because there's no "global" or system build options to conflict with, and everything is easily modifiable in a UI. It looks like omitting Premake's OptimizeSpeed
and ExtraWarnings
flags are the only things that would happen on Windows with the current patch. (I doubt anyone would even try to use the option, but still...)
follow-up: 17 comment:16 by , 12 years ago
Keywords: | review removed |
---|
Replying to historic_bruno:
About the
premake4.lua
patch: line 338 inproject_create()
looks redundant because the flag is already set a few lines earlier inproject_set_build_flags()
. I'm not sure what was intended.
I looked into this a bit more. The problem is that gcc.lua will always emit either -g or -s flags depending on whether Premake's Symbols flag is used (Symbols
=> g++ -g, else => ld -s) and that's not something we control directly with premake4.lua
. We'd need some other means to disable both flags: the yucky way using sed or the nicer way of adding a new flag like NoSymbolsFlags
:)
Anyway they are not a serious problem IMO.
comment:17 by , 12 years ago
Replying to historic_bruno:
the yucky way using sed or the nicer way of adding a new flag like
NoSymbolsFlags
:)
In this case I'd opt for the NoSymbolsFlags
way. ;)
Anyway they are not a serious problem IMO.
Agreed.
comment:19 by , 12 years ago
Milestone: | Alpha 11 → Backlog |
---|
comment:20 by , 10 years ago
Keywords: | patch removed |
---|---|
Milestone: | Backlog → Alpha 11 |
Resolution: | → fixed |
Status: | new → closed |
This was fixed a long time ago.
Can you be more specific, which flags are especially problematic? What are the system flags you expect to be used in this case?