Bug#545794: a proposed patch

[Eugene V. Lyubimkin]

Hi Junichi,

Junichi Uekawa wrote:
> 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 ?
Because unless I do that, /usr/lib/pbuilder/pbuilder-checkparams will fail to
recognize '--cupt' param, as it will see only the name of .dsc to build.

>> 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.
Well, I don't have to, however I will. But anyway, what's wrong with current
variant?

> 
>> +		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...
Why have I? Extra lines? Cupt is other package manager, and though it has
similar commands and options, but it doesn't need to follow apt-get precisely.

>> 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.
However you have several other places in code with 'sh -c' here and there.

> I guess you should fix cupt.
Why? I will probably implement '-y' switch later, but again, I see nothing
wrong in having "echo 'y'", at least in starter patch.


-- 
Eugene V. Lyubimkin aka JackYF, JID: jackyf.devel(maildog)gmail.com
C++/Perl developer, Debian Developer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/pbuilder-maint/attachments/20091001/4089230d/attachment.pgp>