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

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to