widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

Note to self: assuming the recent changes are ok, the remaining points are:
- link to somewhere on the wiki which explains manual building in more detail
- fix indentation and creation of update.sh


Diff comments:

> === modified file 'compile.sh'
> --- compile.sh	2014-06-18 17:11:08 +0000
> +++ compile.sh	2014-07-19 14:18:30 +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,94 +48,51 @@
>        echo "  source code."
>        exit 1
>      fi
> +    #TODO(code review): Are these returns in various methods necessary?

I just left them in. Probably not needed, but I'm not an expert on shell scripting either.

> +    #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 () {
> +    #Defaults to ninja, but if that is not found, we use make instead
> +    if [ `command -v ninja` ] ; then
> +      buildtool="ninja"
> +    #On some systems (most notably Fedora), the binary is called ninja-build
> +    elif [ `command -v ninja-build` ] ; then
> +      buildtool="ninja-build"
> +    else
> +      buildtool="make"

Right, I had forgotten about that. Added it now.

Though in the cases where GNU make is known as gmake, isn't there usually some other make binary available which should work just as well?

>      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
>  
> -    cd build
> -
>      return 0
>    }
>  
>    # 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, "

Yes, that would make sense. I just felt I should add _some_ explanation, but your suggestion is better.

> +    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
> +
> +    if [ $buildtool = "ninja" ] || [ $buildtool = "ninja-build" ] ; 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 +106,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"

I'll experiment a bit. Worst-case scenario it can be a multiline echo, which I thought a bit about.

>              echo "echo \" \""
>              echo "echo \"################################################\""
> @@ -181,12 +129,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 +147,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 +157,20 @@
>  ######################################
>  set -e
>  basic_check
> -user_interaction
> +set_buildtool
>  prepare_directories_and_links
> +cd build
>  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 "#####################################################"
>  ######################################
> 


-- 
https://code.launchpad.net/~widelands-dev/widelands/trimming-compile/+merge/227390
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/trimming-compile.