widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

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?
> +    #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"
>      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, "
> +    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

I plan to reduce the indentation used below, though for the moment it is much easier to see the actual changes.

> -    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 +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 requested to review the proposed merge of lp:~widelands-dev/widelands/trimming-compile into lp:widelands.