Simplify compile.sh and make it perform better

Bug #1343281 reported by SirVer
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
widelands
Fix Released
Medium
Hans Joachim Desserud

Bug Description

There are a bunch of issues around compile.sh and I think it is time to kill it.

1) compile.sh was made as a bandaid to make it "easy" for users to compile widelands.
The main problem it fixed though was that our builds were not incrementally correct when we did not run cmake everytime. This was because we used glob() to figure out what files to compile - if a new file was added, cmake would need to remake the build files, but since we never touched a CmakeFile it could not do it automatically.

This problem is gone now: each source is mentioned in a CMakeLists.txt file and adding or removing one will need to modify the CmakeLists.txt file too - and cmake reruns itself. So running 'make' in the build directory is now always sufficient and usually way faster.

2) compile.sh uses MakeFiles and make to build the system. This is noisy (e.g. bug 1342741) and slow. Using cmake -G Ninja and 'ninja' [1] is ~100 times faster.

[1] http://martine.github.io/ninja/

3) All in all, building widelands without compile.sh is as easy as:

mkdir build && cd build
cmake -G Ninja -DCMAKE_BUILD_TYPE:STRING="Release" ..
ninja

Updating it:

bzr pull && cd build && ninja

Tags: buildsystem

Related branches

SirVer (sirver)
Changed in widelands:
status: New → Incomplete
Revision history for this message
Jens Beyer (qcumber-some) wrote :

Hmm. I guess those are too many things in one bug.

compile.sh and make/ninja are independend.

I also believe ninja could be a good replacement for make. I would still not think it should "replace" compile.sh.
One of the major things of compile.sh is the update.sh which makes it easier for bzr illiterates (non-devs).
Then, even now, cmake/make is as easy as cmake/ninja. There is virtually no difference for me.

Then: update.sh runs "touch CMakeLists.txt" for a long time now, which forces cmake to run again. Of course this is slower, but it is always correct. It even makes make lang pick up new catalogs.

So, from my point of view:
+1 for ninja
-1 for removing compile.sh

The bug should be separated into "implement ninja support" and "move compile.sh to ninja".
Question would then be: do we really want to remove support for make (officially) and switch over to ninja? Do we want to support both? What is with the PPA builds?

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

I agree with Jens, this bug sounds like two things:
1. Compile.sh has problems and can be replaced with simpler commands so a user only really have to run "cmake .. && make" and "bzr pull && make". That sounds like a reasonable goal, and something which should be pursued.
2. Switch to ninja. I'm not really opposed to this. (From what I can see it has been built as a drop-in replacement for make, so I don't see any obvious reasons why WL wouldn't work with either similar to how it is buildable with GCC or Clang/LLVM at the present.)

I wasn't really around at the time it was introduced (I think...) but compile.sh always struck me as a way to hide part of the complexity, for instance arguments sent to cmake. My ideal version would be just a few simple straight-forward commands for people to invoke with as few arguments as possible. So I'm not too happy that using cmake would add a new argument for people to deal with. (I don't have full overview over how widespread packaging of ninja is either, see below). This _might_ (and I haven't decided myself) may make a case for keeping make as the default, though document steps for ninja as an alternative.

Then there's also the coice between release and debug build which needs to be specified when calling cmake (unless we set release as the default, though allow people to override it).

Another thing is that compile.sh asks whether translations should be built which happens when invoking `make lang`. So presumable for anyone who wish to play in a different language would need to run that command in addition to the initial "cmake && make".

So we end up with three options or diverging paths; makesystem, build type and with/without translations. I haven't checked but would `make all` include translations as well, or would we need to tweak something to get translations by default.

A quick comment regarding the PPA builds; they are built from scratch each time so won't really gain any benefit from faster upgrades, though presumably the general speed-ups. Furthermore, ninja won't be available on 12.04 (https://launchpad.net/ubuntu/+source/ninja-build. I don't know whether it is/how recently it was packaged on other systems.

tags: added: buildsystem cmake
Revision history for this message
SirVer (sirver) wrote :

I changed the bug. I think these issues cannot be completely separated: building lang takes long with gnu make because it has to look at so many targets, if we build this always by default, gnu make is no longer convenient for users.

I also think we should not glob() for language files, but instead have a CMakeLists.txt that lists all of them, so that Cmake can correctly rebuild them. touch CMakeLists.txt is a dirty work around.

My suggestions:

1) Make ninja the default in compile.sh (and update the documentation).
2) always build lang, always create update.sh without asking.
3) Open a new bug that looks at how we can get rid of globbing also for lang files.

summary: - Get rid of compile.sh or move it to Ninja
+ Simplify compile.sh and make it perform better
Revision history for this message
Hans Joachim Desserud (hjd) wrote :

I'll start at trimming down compile.sh.

One question though, should `make lang` still be a separate option or simply included as a part of `make`? Because I don't know how these targets are defined.

PS. s/ninja/make/ is left as an exercise for the reader ;p

Changed in widelands:
assignee: nobody → Hans Joachim Desserud (hjd)
importance: Undecided → Medium
milestone: none → build19-rc1
status: Incomplete → Confirmed
Revision history for this message
SirVer (sirver) wrote :

Fix for this was merged in r7142.

Changed in widelands:
status: Confirmed → Fix Committed
Revision history for this message
GunChleoc (gunchleoc) wrote :

Thanks :)

GunChleoc (gunchleoc)
tags: removed: cmake
GunChleoc (gunchleoc)
Changed in widelands:
status: Fix Committed → Fix Released
Revision history for this message
GunChleoc (gunchleoc) wrote :

Fixed in build19-rc1.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.