Bug#545794: a proposed patch

[Junichi Uekawa]

Hi,

I've looked through this patch. I have a few questions for you.

> diff -urN pbuilder-0.189/pbuilder-buildpackage pbuilder-0.189+jackyf1/pbuilder-buildpackage
> --- pbuilder-0.189/pbuilder-buildpackage	2009-05-31 04:41:04.000000000 +0300
> +++ pbuilder-0.189+jackyf1/pbuilder-buildpackage	2009-09-27 22:02:57.288519614 +0300
> @@ -21,11 +21,13 @@
>  export LC_ALL=C
>  set -e
>  
> +PACKAGENAME=$1
> +shift
> +
>  . /usr/lib/pbuilder/pbuilder-checkparams
>  . /usr/lib/pbuilder/pbuilder-runhooks
>  . /usr/lib/pbuilder/pbuilder-buildpackage-funcs
>  
> -PACKAGENAME="$1"
>  if [ ! -f "$PACKAGENAME" ]; then
>      log "E: Command line parameter [$PACKAGENAME] is not a valid .dsc file name"
>      exit 1;

Why ?

> diff -urN pbuilder-0.189/pbuilder-buildpackage-funcs pbuilder-0.189+jackyf1/pbuilder-buildpackage-funcs
> --- pbuilder-0.189/pbuilder-buildpackage-funcs	2009-02-26 07:33:11.000000000 +0200
> +++ pbuilder-0.189+jackyf1/pbuilder-buildpackage-funcs	2009-09-27 22:37:20.494949826 +0300
> @@ -37,6 +37,11 @@
>  	yes) BUILDOPT="--binary-arch";;
>  	*) ;;
>      esac
> +
> +	if [ -n "$USE_CUPT" ]; then
> +		PBUILDERSATISFYDEPENDSCMD=/usr/lib/pbuilder/pbuilder-satisfydepends-cupt
> +	fi
> +
>      if "$PBUILDERSATISFYDEPENDSCMD" --control "$1" --chroot "${BUILDPLACE}" --internal-chrootexec "${CHROOTEXEC}" "${BUILDOPT}" ; then
>  	:
>      else
> @@ -50,7 +55,12 @@
>      fi
>      # install extra packages to the chroot
>      if [ -n "$EXTRAPACKAGES" ]; then 
> -	$CHROOTEXEC usr/bin/apt-get -y --force-yes install ${EXTRAPACKAGES}
> +		if [ -n "$USE_CUPT" ]; then
> +			PACKAGE_MANAGER_COMMAND="echo 'y' | /usr/bin/cupt --no-auto-remove"
I think you should fix cupt to accept -y.

> +		else
> +			PACKAGE_MANAGER_COMMAND="/usr/bin/apt-get -y --force-yes"
> +		fi
> +		$CHROOTEXEC sh -c "$PACKAGE_MANAGER_COMMAND install ${EXTRAPACKAGES}"
>      fi
>  }

I'd rather have cupt wrapper accepting same command-line as apt-get than adding if's here...
> diff -urN pbuilder-0.189/pbuilder-updatebuildenv pbuilder-0.189+jackyf1/pbuilder-updatebuildenv
> --- pbuilder-0.189/pbuilder-updatebuildenv	2009-06-19 17:24:13.000000000 +0300
> +++ pbuilder-0.189+jackyf1/pbuilder-updatebuildenv	2009-09-27 22:00:41.283468835 +0300
> @@ -34,28 +34,46 @@
>  extractbuildplace
>  $TRAP umountproc_cleanbuildplace_trap exit sighup
>  
> +recover_aptcache
> +
> +if [ -n "$USE_CUPT" ]; then
> +	if ! $CHROOTEXEC /usr/bin/dpkg -l | grep "^ii" | grep cupt; then
> +		log "I: installing cupt package manager";
> +		$CHROOTEXEC apt-get update
> +		$CHROOTEXEC apt-get -y install cupt
> +	fi
> +	PACKAGE_MANAGER=/usr/bin/cupt
> +	PACKAGE_MANAGER_COMMAND="echo 'y' | /usr/bin/cupt -o apt::get::allowunauthenticated=1"
> +else
> +	PACKAGE_MANAGER=/usr/bin/apt-get
> +	PACKAGE_MANAGER_COMMAND="/usr/bin/apt-get -y --force-yes"
> +fi
> +
>  loadhooks
>  log "I: Refreshing the base.tgz "
>  log "I: upgrading packages"
> -$CHROOTEXEC /usr/bin/apt-get update
> +$CHROOTEXEC $PACKAGE_MANAGER update
>  if [ -n "$REMOVEPACKAGES" ]; then
>      $CHROOTEXEC /usr/bin/dpkg --purge $REMOVEPACKAGES
>  fi
> -recover_aptcache
>  
>  $TRAP saveaptcache_umountproc_cleanbuildplace_trap exit sighup
> -$CHROOTEXEC /usr/bin/apt-get -y --force-yes "${FORCE_CONFNEW[@]}" dist-upgrade
> +$CHROOTEXEC sh -c "$PACKAGE_MANAGER_COMMAND -o DPkg::Options::=--force-confnew dist-upgrade"

I don't generally like this change to 'sh -c' because it will need another layer
of having to care about quoting.

I guess you should fix cupt.

>  # autoremove: Ignore error in case of etch because apt in etch doesn't
>  # support autoremove. TODO: Do not ignore error when etch is no longer
>  # supported.
> -$CHROOTEXEC /usr/bin/apt-get -y --force-yes autoremove || true
> -$CHROOTEXEC /usr/bin/apt-get -y --force-yes install build-essential dpkg-dev apt aptitude $EXTRAPACKAGES
> +if [ -z "$USE_CUPT" ]; then
> +	$CHROOTEXEC /usr/bin/apt-get -y --force-yes autoremove || true
> +fi
> +
> +FULL_COMMAND="$PACKAGE_MANAGER_COMMAND install build-essential dpkg-dev apt aptitude $EXTRAPACKAGES"
> +$CHROOTEXEC sh -c "$FULL_COMMAND"
>  save_aptcache
>  
>  # optionally auto-clean apt-cache
>  if [ "${AUTOCLEANAPTCACHE}" = "yes" -a -n "$APTCACHE" ]; then
>      log "I: Cleaning the cached apt archive"
> -    $CHROOTEXEC /usr/bin/apt-get autoclean || true
> +    $CHROOTEXEC $PACKAGE_MANAGER autoclean || true
>      find "$APTCACHE/" -maxdepth 1 -name \*.deb | \
>  	while read A; do
>  	    if [ ! -f "$BUILDPLACE/var/cache/apt/archives/"$(basename "$A") -a \
> @@ -71,7 +89,7 @@
>  unloadhooks
>  
>  umountproc
> -$CHROOTEXEC /usr/bin/apt-get clean || true
> +$CHROOTEXEC $PACKAGE_MANAGER clean || true
>  
>  $TRAP cleanbuildplace_trap exit sighup
>  if [ ! "${INTERNAL_BUILD_UML}" = "yes" ]; then