[PATCH] premake adds random flags

[hasufell]

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.

[historic_bruno]

Can you be more specific, which flags are especially problematic? What are the system flags you expect to be used in this case?

[hasufell]

-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...

[leper]

In 12025:

Don't add system CFLAGS to the CXXFLAGS. Refs #1516.

[leper]

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.

[leper]

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.

[historic_bruno]

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.

[leper]

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?

[historic_bruno]

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)

[hasufell]

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.

[hasufell]

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.

[hasufell]

fixed compilation for -O2 and -O3 in FCollada, so we don't have to force -O1 anymore

[hasufell]

actually this still makes trouble with -O3...

[hasufell]

final fix for -O3

[leper]

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.)

[Philip]

r12072 should make the fcollada/src/Makefile CXXFLAGS change unnecessary - it should build fine with the default -O2/-O3 behaviour.

[pcpa]

Rediffed as I understood it, for (late) feedback of #1421 comment, and it works in not adding rpath in a rpm build

[historic_bruno]

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...)

[ben]

In 12173:

Adds --minimal-flags option to Premake, for building only with essential flags (currently only a *nix option). Based on patch by leper. Refs #1516.
Updates Premake4.make to not include CFLAGS in CXXFLAGS (see r12025).

[historic_bruno]

Replying to historic_bruno:

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.

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.

[leper]

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.

[ben]

In 12198:

Fixes incorrect exclusion of rpath that broke shared lib loading. Refs #1421, #1516