Thanks for reviewing, Shevonar! I am very grateful that you took up reviewing 
changes.

> It seems that the WL_SRC set contains most of the source files twice: once 
> with indentation, once without.

Every file was only there once, but the indent was not consistent. Fixed.

[minizip]
I much rather always use a upstream version, but most distributions do not 
offer one. Instead of sometimes doing this or sometimes doing anything else, I 
much prefer we just use the bundled one and be done with. If some upstream 
decides to change the shipped headers of zlib and this breaks us, then we need 
extra patches for those systems - or we need to bring back the conditional if 
everything else fails.

[dependencies]
First, SDL is currently linked into everything, so it is strange that it 
complains about missing SDL dependency there.

Here is what I think is happening: we have a bunch of cyclic dependencies in 
the system (this is one of the motivations of this change: make them explicit, 
so they can be broken. this and deterministic builds.). sound for example 
includes files from logic/, so it depends on that. However, linking sound 
itself into a STATIC library does not seem to have any problems with missing 
dependencies - 'ninja sound' just works, though it must contain unresolved 
symbols.

Only when you want to build a .exe or a shared library will the system start 
complaining about missing dependencies. Right now, we only do that with tests 
and widelands.exe. Then, the order of the libraries on the commandline suddenly 
matters - and we might need to include some library more than once to get all 
symbols in one linker pass. I think this is where it fails right now as cmake 
does not seem to be doing that. 

If this problem can be solved by really mentioning all dependencies for each 
static library, I am unsure. There will be cycles there and I do not know what 
cmake makes of that then. All I know is that it compiles and links on my system 
(using the gold linker which seems to do multiple passes), so I cannot really 
move this problem forward.

-- 
https://code.launchpad.net/~widelands-dev/widelands/cmake-reworked/+merge/222455
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/cmake-reworked.

_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Reply via email to