[pacman-dev] [PATCH] makepkg: unify tarball creation

[lolilolicon]

On Fri, Sep 30, 2011 at 1:40 AM, Dave Reisner <d at falconindy.com> wrote:
> On Fri, Sep 30, 2011 at 01:23:37AM +0800, lolilolicon wrote:
>> Introduce ext_to_tar_opt() to unify package and source package tarball
>> creation.  This requires bsdtar to support the compression options -z,
>> -j, -J and -Z.  Note also the 'compress' command is not available in
>> Arch (FS#25908), so 'bsdtar -Z' seems to be the only way to support
>> .tar.Z archive creation.
>>
>> Signed-off-by: lolilolicon <lolilolicon at gmail.com>
>> ---
>>  scripts/makepkg.sh.in |  107 ++++++++++++++++++------------------------------
>>  1 files changed, 40 insertions(+), 67 deletions(-)
>>
>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>> index 86e74a6..75c3730 100644
>> --- a/scripts/makepkg.sh.in
>> +++ b/scripts/makepkg.sh.in
>> @@ -1212,6 +1212,22 @@ check_package() {
>>
>>  }
>>
>> +# get bsdtar compression flag from extension
>> +ext_to_tar_opt() {
>> +     local extension=$1 TAR_OPT=
>> +     case "$extension" in
>> +             *.tar.gz)  TAR_OPT="z" ;;
>> +             *.tar.bz2) TAR_OPT="j" ;;
>> +             *.tar.xz)  TAR_OPT="J" ;;
>> +             *.tar.Z)   TAR_OPT="Z" ;;
>> +             *.tar)                 ;;
>> +             *) warning "$(gettext "'%s' is not a valid archive extension.")" \
>> +             "$extension" ;;
>> +     esac
>> +
>> +     printf '%s' "$TAR_OPT"
>> +}
>> +
>
> You've failed to account for an unsupported/invalid extension being
> passed. We throw a warning, print nothing, and probably end up with a
> tar archive called $pkgname-$pkgver-$pkgrel-$arch.pkg.tar.LOLLERCAUST.
>

I don't see any problem if that's what the user wants- a funny name maybe,
a tar nontheless, no?

>>  create_package() {
>>       if [[ ! -d $pkgdir ]]; then
>>               error "$(gettext "Missing %s directory.")" "pkg/"
>> @@ -1253,71 +1269,43 @@ create_package() {
>>               fi
>>       done
>>
>> -     # tar it up
>> -     msg2 "$(gettext "Compressing package...")"
>> -
>> -     local EXT
>> -     case "$PKGEXT" in
>> -             *tar.gz)  EXT=${PKGEXT%.gz} ;;
>> -             *tar.bz2) EXT=${PKGEXT%.bz2} ;;
>> -             *tar.xz)  EXT=${PKGEXT%.xz} ;;
>> -             *tar.Z)   EXT=${PKGEXT%.Z} ;;
>> -             *tar)     EXT=${PKGEXT} ;;
>> -             *) warning "$(gettext "'%s' is not a valid archive extension.")" \
>> -             "$PKGEXT" ; EXT=$PKGEXT ;;
>> -     esac
>> -
>>       local fullver=$(get_full_version)
>>       local pkg_file="$PKGDEST/${nameofpkg}-${fullver}-${PKGARCH}${PKGEXT}"
>> -     local ret=0
>>
>>       [[ -f $pkg_file ]] && rm -f "$pkg_file"
>>       [[ -f $pkg_file.sig ]] && rm -f "$pkg_file.sig"
>>
>> +     # tar it up
>> +     msg2 "$(gettext "Compressing package...")"
>> +     local TAR_OPT=$(ext_to_tar_opt "$PKGEXT")
>>       # when fileglobbing, we want * in an empty directory to expand to
>>       # the null string rather than itself
>>       shopt -s nullglob
>> -     # TODO: Maybe this can be set globally for robustness
>> -     shopt -s -o pipefail
>> -     bsdtar -cf - $comp_files * |
>> -     case "$PKGEXT" in
>> -         *tar.gz)  gzip -c -f -n ;;
>> -         *tar.bz2) bzip2 -c -f ;;
>> -         *tar.xz)  xz -c -z - ;;
>> -         *tar.Z)   compress -c -f ;;
>> -         *tar)     cat ;;
>> -     esac > "${pkg_file}" || ret=$?
>> -
>> -     shopt -u nullglob
>> -     shopt -u -o pipefail
>> -
>> -     if (( ret )); then
>> +     if ! bsdtar -c${TAR_OPT}f "$pkg_file" $comp_files *; then
>>               error "$(gettext "Failed to create package file.")"
>>               exit 1 # TODO: error code
>> -     fi
>> -
>> -     create_signature "$pkg_file"
>> -
>> -     if (( ! ret )) && [[ ! "$PKGDEST" -ef "${startdir}" ]]; then
>> -             rm -f "${pkg_file/$PKGDEST/$startdir}"
>> -             ln -s "${pkg_file}" "${pkg_file/$PKGDEST/$startdir}"
>> -             ret=$?
>> -             if [[ -f $pkg_file.sig ]]; then
>> -                     rm -f "${pkg_file/$PKGDEST/$startdir}.sig"
>> -                     ln -s "$pkg_file.sig" "${pkg_file/$PKGDEST/$startdir}.sig"
>> +     else
>> +             create_signature "$pkg_file"
>> +             if [[ ! "$PKGDEST" -ef "${startdir}" ]]; then
>> +                     rm -f "${pkg_file/$PKGDEST/$startdir}"
>> +                     if ! ln -s "${pkg_file}" "${pkg_file/$PKGDEST/$startdir}"
>> +                             warning "$(gettext "Failed to create symlink to package file.")"
>> +                     fi
>> +                     if [[ -f $pkg_file.sig ]]; then
>> +                             rm -f "${pkg_file/$PKGDEST/$startdir}.sig"
>> +                             if ! ln -s "$pkg_file.sig" "${pkg_file/$PKGDEST/$startdir}.sig"
>> +                                     warning "$(gettext "Failed to create symlink to signature file.")"
>> +                             fi
>> +                     fi
>>               fi
>>       fi
>
> Unrelated changes here... as far as I can see, we gain nothing from
> disturbing this block of code but introducing duplication.

What do you mean by duplication? This just eliminates the use of $ret...
Did this because I noticed ret was used uninitialized, but yes it's unrelated...

>
>> -
>> -     if (( ret )); then
>> -             warning "$(gettext "Failed to create symlink to package file.")"
>> -     fi
>> +     shopt -u nullglob
>>  }
>>
>>  create_signature() {
>>       if [[ $SIGNPKG != 'y' ]]; then
>>               return
>>       fi
>> -     local ret=0
>>       local filename="$1"
>>       msg "$(gettext "Signing package...")"
>>
>> @@ -1326,10 +1314,7 @@ create_signature() {
>>               SIGNWITHKEY="-u ${GPGKEY}"
>>       fi
>>       # The signature will be generated directly in ascii-friendly format
>> -     gpg --detach-sign --use-agent ${SIGNWITHKEY} "$filename" &>/dev/null || ret=$?
>> -
>> -
>> -     if (( ! ret )); then
>> +     if gpg --detach-sign --use-agent ${SIGNWITHKEY} "$filename" &>/dev/null; then
>>               msg2 "$(gettext "Created signature file %s.")" "$filename.sig"
>>       else
>>               warning "$(gettext "Failed to sign package file.")"
>> @@ -1369,35 +1354,23 @@ create_srcpackage() {
>>               done < <(sed -n "s/^[[:space:]]*$i=//p" "$BUILDFILE")
>>       done
>>
>> -     local TAR_OPT
>> -     case "$SRCEXT" in
>> -             *tar.gz)  TAR_OPT="z" ;;
>> -             *tar.bz2) TAR_OPT="j" ;;
>> -             *tar.xz)  TAR_OPT="J" ;;
>> -             *tar)     TAR_OPT=""  ;;
>> -             *) warning "$(gettext "'%s' is not a valid archive extension.")" \
>> -             "$SRCEXT" ;;
>> -     esac
>> -
>>       local fullver=$(get_full_version)
>>       local pkg_file="$SRCPKGDEST/${pkgbase}-${fullver}${SRCEXT}"
>>
>>       # tar it up
>>       msg2 "$(gettext "Compressing source package...")"
>>       cd "${srclinks}"
>> +     local TAR_OPT=$(ext_to_tar_opt "$SRCEXT")
>>       if ! bsdtar -c${TAR_OPT}Lf "$pkg_file" ${pkgbase}; then
>>               error "$(gettext "Failed to create source package file.")"
>>               exit 1 # TODO: error code
>>       fi
>>
>> -     if (( ! ret )) && [[ ! "$SRCPKGDEST" -ef "${startdir}" ]]; then
>> +     elif [[ ! "$SRCPKGDEST" -ef "${startdir}" ]]; then
>>               rm -f "${pkg_file/$SRCPKGDEST/$startdir}"
>> -             ln -s "${pkg_file}" "${pkg_file/$SRCPKGDEST/$startdir}"
>> -             ret=$?
>> -     fi
>> -
>> -     if (( ret )); then
>> -             warning "$(gettext "Failed to create symlink to source package file.")"
>> +             if ! ln -s "${pkg_file}" "${pkg_file/$SRCPKGDEST/$startdir}"; then
>> +                     warning "$(gettext "Failed to create symlink to source package file.")"
>> +             fi
>>       fi
>
> More churn, here...
>
>>
>>       cd "${startdir}"
>> --
>> 1.7.6.4
>>
>>
>
>