widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

Hans Joachim Desserud has proposed merging lp:~widelands-dev/widelands/trimming-compile into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1343281 in widelands: "Simplify compile.sh and make it perform better"
  https://bugs.launchpad.net/widelands/+bug/1343281

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/trimming-compile/+merge/227390

Trim down/simplify compile.sh:
* Removed all interactivity:
  * Selects "Debug"
  * Always builds translations
  * Always creates an update script
* Switch to ninja by default (though allow fallback to make if ninja is not available)
* Various cleanup/simplification

I've updated most of the dependency lists for distributions on https://wl.widelands.org/wiki/Building%20Widelands/ to now include ninja-packages. The remaining ones are Mandriva and Mageia, neither of which I know well enough to know where to search for available packages.

The other thing is whether we can relay on the ninja exectuable always being "ninja". I would need to test on a Fedora system to verify, but at a glance it looks like the package might install it as ninja-build. 

See commit messages for details, and TODOs for remaining questions/issues.
-- 
https://code.launchpad.net/~widelands-dev/widelands/trimming-compile/+merge/227390
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/trimming-compile into lp:widelands.

=== modified file 'compile.sh'
--- compile.sh	2014-06-18 17:11:08 +0000
+++ compile.sh	2014-07-18 19:54:15 +0000
@@ -26,15 +26,14 @@
 # TODO  user interaction and functions for installation including a check
 # TODO  whether the selected directories are writeable and a password check
 # TODO  to become root / Administrator if the dirs are not writeable.
+# TODO(code review): probably no longer needed?
 
 
 
 ######################################
 # Definition of some local variables #
 ######################################
-var_build=0 # 0 == debug(default), 1 == release
-var_build_lang=0 # 0 = false 
-var_updater=0 # 0 = false
+buildtool="" #Use ninja by default, fall back to make if that is not available.
 ######################################
 
 
@@ -49,74 +48,26 @@
       echo "  source code."
       exit 1
     fi
+    #TODO(code review): Are these returns in various methods necessary?
+    #It doesn't seem like anything ever checks the return, and in case
+    #of something bad, it just calls exit anyways (see above)
     return 0
   }
 
-  # Ask the user what parts and how Widelands should be build.
-  # And save the values
-  user_interaction () {
-    local_var_ready=0
-    while [ $local_var_ready -eq 0 ]
-    do
-      echo " "
-      echo "  Should Widelands be build in [r]elease or [d]ebug mode?"
-      echo " "
-      read local_var_choice
-      echo " "
-      case $local_var_choice in
-        r) echo "  -> Release mode selected" ; var_build=1 ; local_var_ready=1 ;;
-        d) echo "  -> Debug mode selected" ; var_build=0 ; local_var_ready=1 ;;
-        *) echo "  -> Bad choice. Please try again!" ;;
-      esac
-    done
-    local_var_ready=0
-    if [ $var_build -eq 0 ] ; then
-      while [ $local_var_ready -eq 0 ]
-      do
-        echo " "
-        echo "  Should translations be build [y]/[n]?"
-        echo " "
-        read local_var_choice
-        echo " "
-        case $local_var_choice in
-          y) echo "  -> Translations will be build" ; var_build_lang=1 ; local_var_ready=1 ;;
-          n) echo "  -> Translations will not be build" ; var_build_lang=0 ; local_var_ready=1 ;;
-          *) echo "  -> Bad choice. Please try again!" ;;
-        esac
-      done
+  set_buildtool () {
+    #If ninja is not found, use make instead
+    if [ `command -v ninja` ] ; then
+      buildtool="ninja"
+    #TODO(code review): hopefully ninja has the same executable name across the board...
+    #I'll doublecheck this, but the ninja binary might be called ninja-build on some systems
+    #so will need to check for and use that.
+    else
+      buildtool="make"
     fi
-    return 0
   }
 
   # Check if directories / links already exists and create / update them if needed.
   prepare_directories_and_links () {
-    # remove build/compile directory (this is the old location)
-    if [ -e build/compile ] ; then
-      echo " "
-      echo "  The build directory has changed"
-      echo "  from ./build/compile to ./build."
-      echo "  The old directory ./build/compile can be removed."
-      echo "  Please backup any files you might not want to lose."
-      echo "  Most users can safely say yes here."
-      echo "  Do you want to remove the directory ./build/compile? [y]es/[n]o"
-      echo " "
-      read local_var_choice
-      echo " "
-      case $local_var_choice in
-        y) echo "  -> Removing directory ./build/compile. This may take a while..."
-	   rm locale
-	   rm -r build/compile || true
-	   if [ -e build/compile ] ; then
-             echo "  -> Directory could not be removed. This is not fatal, continuing."
-	   else
-             echo "  -> Directory removed."
-	   fi ;;
-        n) echo "  -> Left the directory untouched." ;;
-        *) echo "  -> Bad choice. Please try again!" ;;
-      esac
-    fi
-
-    test -d build || mkdir -p build
     test -d build/locale || mkdir -p build/locale
     test -e locale || ln -s build/locale
 
@@ -127,16 +78,24 @@
 
   # Compile Widelands
   compile_widelands () {
-    var_build_type=""
-    if [ $var_build -eq 0 ] ; then
-      var_build_type="Debug"
+    echo "Builds a debug build by default. If you want a Release build, "
+    echo "you will need to build it manually passing the"
+    echo "option -DCMAKE_BUILD_TYPE=\"Release\" to cmake"
+
+    #TODO(code review): WL_PORTABLE might be going away, see https://bugs.launchpad.net/widelands/+bug/1342228
+    #TODO(hjd): Remember to upgrade list of build dependencies for various platforms to make sure ninja
+    #is present. Also, someone should do some research how available it is on various other platforms.
+
+    if [ $buildtool = "ninja" ] ; then
+      cmake -G Ninja -DWL_PORTABLE=true .. -DCMAKE_BUILD_TYPE="Debug"
     else
-      var_build_type="Release"
+      cmake -DWL_PORTABLE=true .. -DCMAKE_BUILD_TYPE="Debug"
     fi
-
-    echo " "
-    cmake -DWL_PORTABLE=true .. -DCMAKE_BUILD_TYPE="${var_build_type}"
-    make ${MAKEOPTS}
+      #TODO(code review): Do we need to pass in makeopts, is it likely that people running this script will
+      #have makeopts set? Also, I assume ninja will be able to deal with this, if it is a drop-in replacement
+      $buildtool ${MAKEOPTS}
+      #TODO(code review): Ideally lang is always run as just another part of the line above
+      $buildtool lang
     return 0
   }
 
@@ -150,22 +109,14 @@
     return 0
   }
 
-  # Ask the user whether an update script should be created and if yes, create it.
-  update_script () {
+  create_update_script () {
     # First check if this is an bzr checkout at all - only in that case,
     # creation of a script makes any sense.
     if ! [ -f .bzr/branch-format ] ; then
+      echo "You don't appear to be using Bazaar. An update script will not be created"
       return 0
     fi
-    while :
-    do
-      echo " "
-      echo "  Should I create an update script? [y]es/[n]o"
-      echo " "
-      read local_var_choice
-      echo " "
-      case $local_var_choice in
-        y) rm -f update.sh || true
+        rm -f update.sh || true
            (echo "#!/bin/sh"
             echo "echo \" \""
             echo "echo \"################################################\""
@@ -181,12 +132,10 @@
             echo "fi"
             echo " "
             echo "bzr pull"
-            echo "touch CMakeLists.txt"
             echo "cd build"
-            echo "make ${MAKEOPTS}"
-            if [ $var_build_lang -eq 1 ] ; then
-              echo "make lang"
-            fi
+            echo "cmake .."
+            echo "$buildtool ${MAKEOPTS}"
+            echo "$buildtool lang"
             echo "rm  ../VERSION || true"
             echo "rm  ../widelands || true"
             echo "mv VERSION ../VERSION"
@@ -201,11 +150,6 @@
            ) > update.sh
            chmod +x ./update.sh
            echo "  -> The update script has successfully been created."
-           var_updater=1 ; return 0 ;;
-        n) echo "  -> No update script has been created." ; return 0 ;;
-        *) echo "  -> Bad choice. Please try again!" ;;
-      esac
-    done
   }
 ######################################
 
@@ -216,24 +160,19 @@
 ######################################
 set -e
 basic_check
-user_interaction
+set_buildtool
 prepare_directories_and_links
 compile_widelands
-if [ $var_build_lang -eq 1 ] ; then
-  make lang
-fi
 move_built_files
 cd ..
-update_script
+create_update_script
 echo " "
 echo "#####################################################"
 echo "# Congratulations Widelands was successfully build. #"
 echo "# You should now be able to run Widelands via       #"
 echo "# typing ./widelands + ENTER in your terminal       #"
-if [ $var_updater -eq 1 ] ; then
-  echo "#                                                   #"
-  echo "# You can update Widelands via running ./update.sh  #"
-  echo "# in the same directory you ran this script in.     #"
-fi
+echo "#                                                   #"
+echo "# You can update Widelands via running ./update.sh  #"
+echo "# in the same directory you ran this script in.     #"
 echo "#####################################################"
 ######################################