widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing



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

these TODOs all seem like a bad idea to me. We should not do anything that requires root and the rest seem like they were never needed in the last few years.

>  # 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?

probably not. But my bash_foo is pretty weak.

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

check for gmake before make. Some BSD* call it that way.

>      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, "

I would prefer point to a wiki page (paragraph?) and explain the manual building in a little more details.

> +    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

for now it is still around though. So just keep it.

> +
> +    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}

I assume the idea of the compile.sh script is to be as simple as possible for users. If that is the case, I'd rather not pass through some option that the user has likely not set. +1 for removing MAKEOPTS here.

> +    #TODO(code review): Ideally lang is always run as just another part of the line above

this has to be done in cmake then. I think it would make sense to let the widelands executable be dependant on the lang target. Probably not in this merge request though.

> +    $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"

this would be more readable and easier to maintain using cat > update.sh << EOF. Not sure if variables are expanded in that though.

>              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.