[pacman-dev,1/3] makepkg: refactor archive compression for reusability

Message ID 20170829050117.10097-1-eschwartz@archlinux.org
State Superseded, archived
Headers show
Series [pacman-dev,1/3] makepkg: refactor archive compression for reusability | expand

Commit Message

Eli Schwartz Aug. 29, 2017, 5:01 a.m. UTC
This allows for more easily extending the list of allowed compression
methods, as it has to be modified in only one place.

Also allow the user to specify their own preferred command + options for
source packages in addition to compiled packages. Currently,
makepkg.conf(5) erroneously claims this is already possible.

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
---

I'm not 100% sure how to handle this, perhaps it should encompass
bsdtar/$pkg_file as well?

 scripts/makepkg.sh.in | 57 ++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

--
2.14.1

Comments

Allan McRae Sept. 14, 2017, 6:27 a.m. UTC | #1
On 29/08/17 15:01, Eli Schwartz wrote:
> This allows for more easily extending the list of allowed compression
> methods, as it has to be modified in only one place.
> 
> Also allow the user to specify their own preferred command + options for
> source packages in addition to compiled packages. Currently,
> makepkg.conf(5) erroneously claims this is already possible.
> 
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
> 
> I'm not 100% sure how to handle this, perhaps it should encompass
> bsdtar/$pkg_file as well?
> 
>  scripts/makepkg.sh.in | 57 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 20e9dd7e..a5c216dd 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -698,6 +698,26 @@ list_package_files() {
>  	sed -e 's|^\./||' | tr '\n' '\0'
>  }
> 
> +# Wrapper around many stream compression formats, for use in the middle of a
> +# pipeline. A tar archive is passed on stdin and compressed to stdout.
> +compress_as() {

This should really make its way to libmakepkg in util/compress.sh.

I have also internal debated the name for a while.  compress_as() vs
compress_to() vs compress().  I was slightly leaning to the last one...

> +	# $1: final archive filename extension for compression type detection
> +
> +	local ext="$1"
> +
> +	case "$ext" in
> +		*tar.gz)  ${COMPRESSGZ[@]:-gzip -c -f -n} ;;
> +		*tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;;
> +		*tar.xz)  ${COMPRESSXZ[@]:-xz -c -z -} ;;
> +		*tar.lrz) ${COMPRESSLRZ[@]:-lrzip -q} ;;
> +		*tar.lzo) ${COMPRESSLZO[@]:-lzop -q} ;;
> +		*tar.Z)   ${COMPRESSZ[@]:-compress -c -f} ;;

Aside: we need to do something about all these COMPRESSFOO variables...
Adding a new one to our example makepkg.conf with each new addition is
not ideal.  Perhaps we can just document them in the man page but no
have default entries in makepkg.conf?   Or is there a better way?

> +		*tar)     cat ;;
> +		*) warning "$(gettext "'%s' is not a valid archive extension.")" \
> +			"$ext"; cat ;;
> +	esac
> +}
> +
Eli Schwartz Oct. 3, 2017, 2:56 p.m. UTC | #2
On 09/14/2017 02:27 AM, Allan McRae wrote:
>> +# Wrapper around many stream compression formats, for use in the middle of a
>> +# pipeline. A tar archive is passed on stdin and compressed to stdout.
>> +compress_as() {
> 
> This should really make its way to libmakepkg in util/compress.sh.

I suppose I can do that, anything else that should go there or is it a
one-function library?

> I have also internal debated the name for a while.  compress_as() vs
> compress_to() vs compress().  I was slightly leaning to the last one...

I'm not really sure what to call it either. Worth noting though that
compress() will not work -- POSIX has a compress/decompress command for
handling *.Z which is in fact utilized in the function itself, so that
would be rather recursive if anyone tried to use PKGEXT=.pkg.tar.Z for
some outdated reason.

OTOH I cannot find a repository package to provide that currently (but
gzip does provide uncompress as a second copy of the `gunzip -d` wrapper
which is also installed as gunzip). Although the AUR package "ncompress"
appears to be the modern canonical source of a non-patented `compress`
implementation.

So it is essentially dead (or at least AUR) code.

(Gripe: why does this AUR package claim the license for a public
domain/unlicense'd codebase is "GPL".)

>> +	# $1: final archive filename extension for compression type detection
>> +
>> +	local ext="$1"
>> +
>> +	case "$ext" in
>> +		*tar.gz)  ${COMPRESSGZ[@]:-gzip -c -f -n} ;;
>> +		*tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;;
>> +		*tar.xz)  ${COMPRESSXZ[@]:-xz -c -z -} ;;
>> +		*tar.lrz) ${COMPRESSLRZ[@]:-lrzip -q} ;;
>> +		*tar.lzo) ${COMPRESSLZO[@]:-lzop -q} ;;
>> +		*tar.Z)   ${COMPRESSZ[@]:-compress -c -f} ;;
> 
> Aside: we need to do something about all these COMPRESSFOO variables...
> Adding a new one to our example makepkg.conf with each new addition is
> not ideal.  Perhaps we can just document them in the man page but no
> have default entries in makepkg.conf?   Or is there a better way?

Well, the makepkg.conf is already rather duplicative of the effort which
goes into its manpage. But I don't think we add new compression methods
all that often. I don't really have a strong opinion on this.

> 
>> +		*tar)     cat ;;
>> +		*) warning "$(gettext "'%s' is not a valid archive extension.")" \
>> +			"$ext"; cat ;;
>> +	esac
>> +}
>> +

Patch

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 20e9dd7e..a5c216dd 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -698,6 +698,26 @@  list_package_files() {
 	sed -e 's|^\./||' | tr '\n' '\0'
 }

+# Wrapper around many stream compression formats, for use in the middle of a
+# pipeline. A tar archive is passed on stdin and compressed to stdout.
+compress_as() {
+	# $1: final archive filename extension for compression type detection
+
+	local ext="$1"
+
+	case "$ext" in
+		*tar.gz)  ${COMPRESSGZ[@]:-gzip -c -f -n} ;;
+		*tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;;
+		*tar.xz)  ${COMPRESSXZ[@]:-xz -c -z -} ;;
+		*tar.lrz) ${COMPRESSLRZ[@]:-lrzip -q} ;;
+		*tar.lzo) ${COMPRESSLZO[@]:-lzop -q} ;;
+		*tar.Z)   ${COMPRESSZ[@]:-compress -c -f} ;;
+		*tar)     cat ;;
+		*) warning "$(gettext "'%s' is not a valid archive extension.")" \
+			"$ext"; cat ;;
+	esac
+}
+
 create_package() {
 	(( NOARCHIVE )) && return

@@ -748,21 +768,8 @@  create_package() {
 	msg2 "$(gettext "Compressing package...")"
 	# TODO: Maybe this can be set globally for robustness
 	shopt -s -o pipefail
-	# bsdtar's gzip compression always saves the time stamp, making one
-	# archive created using the same command line distinct from another.
-	# Disable bsdtar compression and use gzip -n for now.
 	list_package_files | LANG=C bsdtar -cnf - --null --files-from - |
-	case "$PKGEXT" in
-		*tar.gz)  ${COMPRESSGZ[@]:-gzip -c -f -n} ;;
-		*tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;;
-		*tar.xz)  ${COMPRESSXZ[@]:-xz -c -z -} ;;
-		*tar.lrz) ${COMPRESSLRZ[@]:-lrzip -q} ;;
-		*tar.lzo) ${COMPRESSLZO[@]:-lzop -q} ;;
-		*tar.Z)   ${COMPRESSZ[@]:-compress -c -f} ;;
-		*tar)     cat ;;
-		*) warning "$(gettext "'%s' is not a valid archive extension.")" \
-			"$PKGEXT"; cat ;;
-	esac > "${pkg_file}" || ret=$?
+		compress_as "$PKGEXT" > "${pkg_file}" || ret=$?

 	shopt -u -o pipefail

@@ -843,26 +850,20 @@  create_srcpackage() {
 		done
 	done

-	local TAR_OPT
-	case "$SRCEXT" in
-		*tar.gz)  TAR_OPT="-z" ;;
-		*tar.bz2) TAR_OPT="-j" ;;
-		*tar.xz)  TAR_OPT="-J" ;;
-		*tar.lrz) TAR_OPT="--lrzip" ;;
-		*tar.lzo) TAR_OPT="--lzop" ;;
-		*tar.Z)   TAR_OPT="-Z" ;;
-		*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_safe "${srclinks}"
-	if ! LANG=C bsdtar -cL ${TAR_OPT} -f "$pkg_file" ${pkgbase}; then
+
+	# TODO: Maybe this can be set globally for robustness
+	shopt -s -o pipefail
+	LANG=C bsdtar -cLf - ${pkgbase} | compress_as "$SRCEXT" > "${pkg_file}" || ret=$?
+
+	shopt -u -o pipefail
+
+	if (( ret )); then
 		error "$(gettext "Failed to create source package file.")"
 		exit 1 # TODO: error code
 	fi