[Merge] lp:~cwayne18/phablet-tools/clickbuddy-with-sessions into lp:phablet-tools

[Colin Watson]

Review: Needs Fixing

There's bad indentation throughout - it may not be visible in your editor depending on tab stop settings, but it's quite visible in the LP web UI.  Could you fix that?  Other than that, here's a drive-by review.

Diff comments:

> === modified file 'click-buddy'
> --- click-buddy	2014-09-09 14:46:55 +0000
> +++ click-buddy	2014-10-07 19:12:21 +0000
> @@ -37,6 +37,7 @@
>                  (defaults to arch all)
>    --framework   Target click framework, requires a created click chroot
>                  (defaults to ubuntu-sdk-13.10)
> +  --session     Run in a chroot session, requires a created session

According to the code below, it doesn't require a created session; rather, it creates a session for itself.  Perhaps just delete everything from the comma onward?

>  EOF
>  }
>  
> @@ -49,11 +50,13 @@
>  ARCH="all"
>  FRAMEWORK="ubuntu-sdk-13.10"
>  MAINTMODE=""
> +SESSION_NAME=""
> +SESSION_OPTS=""
>  
>  TEST_DIR='tests/autopilot'
>  DEVICE_USER='phablet'
>  
> -ARGS=$(getopt -o s:h -l "maint-mode,extra-deps:,bzr-source:,provision,no-clean,framework:,dir:,arch:,help" -n "$0" -- "$@")
> +ARGS=$(getopt -o s:h -l "maint-mode,extra-deps:,bzr-source:,provision,no-clean,framework:,dir:,arch:,help,session:" -n "$0" -- "$@")
>  
>  if [ $? -ne 0 ] ; then
>      exit 1
> @@ -107,7 +110,14 @@
>  		--maint-mode)
>  			shift
>  			MAINTMODE=1
> +            shift

This extra shift looks wrong (besides being badly indented).  --maint-mode doesn't take an argument according to the getopt invocation above, so won't this break parsing of that option by eating an additional argument?

>  			;;
> +        --session)
> +            shift
> +            SESSION_NAME="$1"
> +            SESSION_OPTS="--session $1"
> +            shift
> +            ;;

Since you aren't using a pre-created session, but rather creating a session entirely internally to this script, I would suggest not requiring the user to pass in a session name.  Instead, you could make up a random session name (not sure what would be the best tool for that, though there are several; maybe uuidgen?  Make sure to add a dependency on whatever you use if it isn't in the Essential set) and just use that.

Also, the "click chroot run" option name is "--session-name" (or "-n" for short), not "--session".  Python's argparse library does allow you to abbreviate options if the abbreviation is unambiguous, but you mustn't rely on this in scripts, because it would break if we ever added a --session-somethingelse option.

>          --)
>              shift
>              break
> @@ -146,29 +156,34 @@
>  	cmake "$SOURCE" $CMAKE_PARAMS
>  	make DESTDIR=$installdir install
>  else
> -	if [ -n "$EXTRA_DEPS" ] && [ -z "$MAINTMODE" ]; then
> -		echo "click chroot still doesn\'t support sessions and deps outside the"
> -		echo "default sdk were requested, you can optionally use click maint"
> -		echo "to install $EXTRA_DEPS by running"
> -		echo "    click chroot -a$ARCH -f$FRAMEWORK maint apt-get update"
> -		echo "    click chroot -a$ARCH -f$FRAMEWORK maint apt-get install $EXTRA_DEPS"
> -		echo
> -		echo "Your chroot will be unclean from then on, so if you want to"
> -		echo "guarantee clean builds the chroot would need to be recreated."
> -		echo
> -		echo "Use the undocumented --maint-mode if you want this automated."
> -		exit 1
> +	if [ -n "$EXTRA_DEPS" ] && [ -z "$SESSION_NAME" ]; then
> +		echo "Please either give a session name, or pass --maint-mode to install in main chroot"
> +        exit 1
>  	fi
>  	cd "$SOURCE"
> +    if [ -n "$EXTRA_DEPS" ] && [ -n "$SESSION_NAME" ];then
> +        click chroot -a$ARCH -f$FRAMEWORK begin-session "$SESSION_NAME"
> +        click chroot -a$ARCH -f$FRAMEWORK maint $SESSION_OPTS apt-get update
> +        click chroot -a$ARCH -f$FRAMEWORK maint $SESSION_OPTS apt-get install --yes $EXTRA_DEPS
> +        click chroot -a$ARCH -f$FRAMEWORK end-session "$SESSION_NAME"
> +    fi

This seems futile.  It'll install the dependencies and then end the session, thus immediately throwing away the work it just did.

>  	if [ -n "$MAINTMODE" ]; then
>  		trap 'click chroot -a$ARCH -f$FRAMEWORK maint apt-get autoremove --yes $EXTRA_DEPS' \
>  		   	EXIT HUP INT TERM
>  		click chroot -a$ARCH -f$FRAMEWORK maint apt-get update
>  		click chroot -a$ARCH -f$FRAMEWORK maint apt-get install --yes $EXTRA_DEPS
>  	fi
> -	click chroot -a$ARCH -f$FRAMEWORK run cmake $CMAKE_PARAMS
> -	click chroot -a$ARCH -f$FRAMEWORK run make
> -    click chroot -a$ARCH -f$FRAMEWORK run make DESTDIR=$installdir install
> +    #start the session if it is requested
> +    if [ -n "$SESSION_NAME" ];then

For consistent style, let's have a space after the "#" and a space between ";" and "then", both here and below.

> +        click chroot -a$ARCH -f$FRAMEWORK begin-session "$SESSION_NAME"
> +    fi
> +	click chroot -a$ARCH -f$FRAMEWORK run $SESSION_OPTS cmake $CMAKE_PARAMS
> +	click chroot -a$ARCH -f$FRAMEWORK run $SESSION_OPTS make
> +    click chroot -a$ARCH -f$FRAMEWORK run $SESSION_OPTS make DESTDIR=$installdir install
> +    #stop the session if it is requested
> +    if [ -n "$SESSION_NAME" ];then
> +        click chroot -a$ARCH -f$FRAMEWORK end-session "$SESSION_NAME"
> +    fi
>  fi
>  
>  if [ ! -f "$installdir/manifest.json" ]; then
> 


-- 
https://code.launchpad.net/~cwayne18/phablet-tools/clickbuddy-with-sessions/+merge/237477
Your team Ubuntu Phablet Team is subscribed to branch lp:phablet-tools.