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_confn...@]}"
>> 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
signature.asc
Description: OpenPGP digital signature
