[arch-projects] [devtools] Minor fix, and cleanups from Parabola
Hi, Here's another batch of changes backported from Parabola's version of devtools. These changes should be pretty uncontroversial. This patchset applies cleanly if applied over the patchsets I've already submitted. It shouldn't be any real trouble to apply them without applying the previous patches, but the context of the diffs has changed enough that it would require resolution of "conflicts". Let me know if you want me to resubmit them with a different base. The first 3 commits deal with strings the user sees; most significantly: fix a bug where makechrootpkg's usage text could be tricked into lying about default values. The next 3 are general code quality improvements to makechrootpkg; they shouldn't actually change anything. The last 2 let common.sh be used in new ways... even if it isn't used those ways in devtools. Let it be included multiple times (like a C header), and let it be used in scripts that `set -u`. -- Happy hacking, ~ Luke Shumaker
From: Luke Shumaker <lukeshu@parabola.nu> This involves extending the signature of lib/common.sh's `stat_busy()`, `lock()`, and `slock()`. The `mesg=$1; shift` in stat_busy even suggests that this is what was originally intended from it. --- arch-nspawn.in | 2 +- archbuild.in | 8 ++++---- archco.in | 2 +- archrelease.in | 2 +- checkpkg.in | 4 ++-- commitpkg.in | 8 ++++---- crossrepomove.in | 10 +++++----- lddd.in | 4 ++-- lib/common.sh | 10 +++++----- makechrootpkg.in | 6 +++--- mkarchroot.in | 2 +- rebuildpkgs.in | 18 +++++++++--------- 12 files changed, 38 insertions(+), 38 deletions(-) diff --git a/arch-nspawn.in b/arch-nspawn.in index 9787415..7481d82 100644 --- a/arch-nspawn.in +++ b/arch-nspawn.in @@ -33,7 +33,7 @@ while getopts 'hC:M:c:' arg; do M) makepkg_conf="$OPTARG" ;; c) cache_dir="$OPTARG" ;; h|?) usage ;; - *) error "invalid argument '$arg'"; usage ;; + *) error "invalid argument '%s'" "$arg"; usage ;; esac done shift $(($OPTIND - 1)) diff --git a/archbuild.in b/archbuild.in index bb2577d..8339aef 100644 --- a/archbuild.in +++ b/archbuild.in @@ -45,13 +45,13 @@ check_root makechrootpkg_args+=("${@:$OPTIND}") if ${clean_first} || [[ ! -d "${chroots}/${repo}-${arch}" ]]; then - msg "Creating chroot for [${repo}] (${arch})..." + msg "Creating chroot for [%s] (%s)..." "${repo}" "${arch}" for copy in "${chroots}/${repo}-${arch}"/*; do [[ -d $copy ]] || continue - msg2 "Deleting chroot copy '$(basename "${copy}")'..." + msg2 "Deleting chroot copy '%s'..." "$(basename "${copy}")" - lock 9 "$copy.lock" "Locking chroot copy '$copy'" + lock 9 "$copy.lock" "Locking chroot copy '%s'" "$copy" subvolume_delete_recursive "${copy}" rm -rf --one-file-system "${copy}" @@ -74,5 +74,5 @@ else pacman -Syu --noconfirm || abort fi -msg "Building in chroot for [${repo}] (${arch})..." +msg "Building in chroot for [%s] (%s)..." "${repo}" "${arch}" exec makechrootpkg -r "${chroots}/${repo}-${arch}" "${makechrootpkg_args[@]}" diff --git a/archco.in b/archco.in index 6088b8e..77cc8c4 100644 --- a/archco.in +++ b/archco.in @@ -6,7 +6,7 @@ m4_include(lib/common.sh) scriptname=${0##*/} if [[ -z $1 ]]; then - echo 'Usage: '$scriptname' <package name>...' + printf 'Usage: %s <package name>...\n' "$scriptname" exit 1 fi diff --git a/archrelease.in b/archrelease.in index c56367f..3b11652 100644 --- a/archrelease.in +++ b/archrelease.in @@ -59,7 +59,7 @@ done known_files=("${known_files[@]/%/@}") for tag in "$@"; do - stat_busy "Copying ${trunk} to ${tag}" + stat_busy "Copying %s to %s" "${trunk}" "${tag}" if [[ -d repos/$tag ]]; then declare -a trash diff --git a/checkpkg.in b/checkpkg.in index da69df2..fbd30d3 100644 --- a/checkpkg.in +++ b/checkpkg.in @@ -70,8 +70,8 @@ for _pkgname in "${pkgname[@]}"; do msg "Sonames differ in $_pkgname!" echo "$diff_output" else - msg "No soname differences for $_pkgname." + msg "No soname differences for %s." "$_pkgname" fi done -msg "Files saved to $TEMPDIR" +msg "Files saved to %s" "$TEMPDIR" diff --git a/commitpkg.in b/commitpkg.in index 6cf562f..d31f6ba 100644 --- a/commitpkg.in +++ b/commitpkg.in @@ -136,7 +136,7 @@ for _arch in ${arch[@]}; do fullver=$(get_full_version $_pkgname) if ! pkgfile=$(find_cached_package "$_pkgname" "$fullver" "${_arch}"); then - warning "Skipping $_pkgname-$fullver-$_arch: failed to locate package file" + warning "Skipping %s: failed to locate package file" "$_pkgname-$fullver-$_arch" skip_arches+=($_arch) continue 2 fi @@ -144,7 +144,7 @@ for _arch in ${arch[@]}; do sigfile="${pkgfile}.sig" if [[ ! -f $sigfile ]]; then - msg "Signing package ${pkgfile}..." + msg "Signing package %s..." "${pkgfile}" if [[ -n $GPGKEY ]]; then SIGNWITHKEY="-u ${GPGKEY}" fi @@ -184,7 +184,7 @@ fi if [[ "${arch[*]}" == 'any' ]]; then if [[ -d ../repos/$repo-i686 && -d ../repos/$repo-x86_64 ]]; then pushd ../repos/ >/dev/null - stat_busy "Removing $repo-i686 and $repo-x86_64" + stat_busy "Removing %s and %s" "$repo-i686" "$repo-x86_64" svn rm -q $repo-i686 svn rm -q $repo-x86_64 svn commit -q -m "Removed $repo-i686 and $repo-x86_64 for $pkgname" @@ -194,7 +194,7 @@ if [[ "${arch[*]}" == 'any' ]]; then else if [[ -d ../repos/$repo-any ]]; then pushd ../repos/ >/dev/null - stat_busy "Removing $repo-any" + stat_busy "Removing %s" "$repo-any" svn rm -q $repo-any svn commit -q -m "Removed $repo-any for $pkgname" stat_done diff --git a/crossrepomove.in b/crossrepomove.in index 0d1945c..14c264e 100644 --- a/crossrepomove.in +++ b/crossrepomove.in @@ -6,7 +6,7 @@ m4_include(lib/common.sh) scriptname=${0##*/} if [[ -z $1 ]]; then - echo 'Usage: '$scriptname' [pkgbase]' + printf 'Usage: %s [pkgbase]\n' "$scriptname" exit 1 fi @@ -40,13 +40,13 @@ setup_workdir pushd $WORKDIR >/dev/null -msg "Downloading sources for ${pkgbase}" +msg "Downloading sources for %s" "${pkgbase}" svn -q checkout -N "${target_svn}" target_checkout mkdir -p "target_checkout/${pkgbase}/repos" svn -q export "${source_svn}/${pkgbase}/trunk" "target_checkout/${pkgbase}/trunk" || die . "target_checkout/${pkgbase}/trunk/PKGBUILD" -msg "Downloading packages for ${pkgbase}" +msg "Downloading packages for %s" "${pkgbase}" for _arch in ${arch[@]}; do if [[ "${_arch[*]}" == 'any' ]]; then repo_arch='x86_64' @@ -60,7 +60,7 @@ for _arch in ${arch[@]}; do done done -msg "Adding ${pkgbase} to ${target_repo}" +msg "Adding %s to %s" "${pkgbase}" "${target_repo}" svn -q add "target_checkout/${pkgbase}" svn -q propset svn:keywords 'Id' "target_checkout/${pkgbase}/trunk/PKGBUILD" svn -q commit -m"${scriptname}: Moving ${pkgbase} from ${source_repo} to ${target_repo}" target_checkout @@ -70,7 +70,7 @@ popd >/dev/null ssh "${server}" "${target_dbscripts}/db-update" || die -msg "Removing ${pkgbase} from ${source_repo}" +msg "Removing %s from %s" "${pkgbase}" "${source_repo}" for _arch in ${arch[@]}; do ssh "${server}" "${source_dbscripts}/db-remove ${source_repo} ${_arch} ${pkgbase}" done diff --git a/lddd.in b/lddd.in index 663d8ae..f01ebf9 100644 --- a/lddd.in +++ b/lddd.in @@ -17,7 +17,7 @@ TEMPDIR=$(mktemp -d --tmpdir lddd-script.XXXX) msg 'Go out and drink some tea, this will take a while :) ...' # Check ELF binaries in the PATH and specified dir trees. for tree in $PATH $libdirs $extras; do - msg2 "DIR $tree" + msg2 "DIR %s" "$tree" # Get list of files in tree. files=$(find $tree -type f ! -name '*.a' ! -name '*.la' ! -name '*.py*' ! -name '*.txt' ! -name '*.h' ! -name '*.ttf' ! \ @@ -46,4 +46,4 @@ done # clean list sort -u $TEMPDIR/pacman.txt >> $TEMPDIR/possible-rebuilds.txt -msg "Files saved to $TEMPDIR" +msg "Files saved to %s" "$TEMPDIR" diff --git a/lib/common.sh b/lib/common.sh index 9303e2e..19aa7de 100644 --- a/lib/common.sh +++ b/lib/common.sh @@ -56,7 +56,7 @@ error() { stat_busy() { local mesg=$1; shift - printf "${GREEN}==>${ALL_OFF}${BOLD} ${mesg}...${ALL_OFF}" >&2 + printf "${GREEN}==>${ALL_OFF}${BOLD} ${mesg}...${ALL_OFF}" "$@" >&2 } stat_done() { @@ -142,7 +142,7 @@ get_full_version() { } ## -# usage : lock( $fd, $file, $message ) +# usage : lock( $fd, $file, $message, [ $message_arguments... ] ) ## lock() { # Only reopen the FD if it wasn't handed to us @@ -152,14 +152,14 @@ lock() { fi if ! flock -n $1; then - stat_busy "$3" + stat_busy "${@:3}" flock $1 stat_done fi } ## -# usage : slock( $fd, $file, $message ) +# usage : slock( $fd, $file, $message, [ $message_arguments... ] ) ## slock() { # Only reopen the FD if it wasn't handed to us @@ -169,7 +169,7 @@ slock() { fi if ! flock -sn $1; then - stat_busy "$3" + stat_busy "${@:3}" flock -s $1 stat_done fi diff --git a/makechrootpkg.in b/makechrootpkg.in index 890a359..5329f76 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -90,14 +90,14 @@ load_vars() { create_chroot() { # Lock the chroot we want to use. We'll keep this lock until we exit. - lock 9 "$copydir.lock" "Locking chroot copy [$copy]" + lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy" if [[ ! -d $copydir ]] || $clean_first; then # Get a read lock on the root chroot to make # sure we don't clone a half-updated chroot slock 8 "$chrootdir/root.lock" "Locking clean chroot" - stat_busy "Creating clean working copy [$copy]" + stat_busy "Creating clean working copy [%s]" "$copy" if is_btrfs "$chrootdir" && ! mountpoint -q "$copydir"; then subvolume_delete_recursive "$copydir" || die "Unable to delete subvolume %s" "$copydir" @@ -118,7 +118,7 @@ create_chroot() { } clean_temporary() { - stat_busy "Removing temporary copy [$copy]" + stat_busy "Removing temporary copy [%s]" "$copy" if is_btrfs "$chrootdir" && ! mountpoint -q "$copydir"; then btrfs subvolume delete "$copydir" >/dev/null || die "Unable to delete subvolume %s" "$copydir" diff --git a/mkarchroot.in b/mkarchroot.in index 21a7727..1265335 100644 --- a/mkarchroot.in +++ b/mkarchroot.in @@ -31,7 +31,7 @@ while getopts 'hC:M:c:' arg; do M) makepkg_conf="$OPTARG" ;; c) cache_dir="$OPTARG" ;; h|?) usage ;; - *) error "invalid argument '$arg'"; usage ;; + *) error "invalid argument '%s'" "$arg"; usage ;; esac done shift $(($OPTIND - 1)) diff --git a/rebuildpkgs.in b/rebuildpkgs.in index 75e7de0..8a141e0 100644 --- a/rebuildpkgs.in +++ b/rebuildpkgs.in @@ -14,8 +14,8 @@ m4_include(lib/common.sh) if (( $# < 1 )); then - echo "Usage: $(basename $0) <chrootdir> <packages to rebuild>" - echo " example: $(basename $0) ~/chroot readline bash foo bar baz" + printf 'Usage: %s <chrootdir> <packages to rebuild>\n' "$(basename "$0")" + printf ' example: %s ~/chroot readline bash foo bar baz\n' "$(basename "$0")" exit 1 fi @@ -51,7 +51,7 @@ pkgs="$@" SVNPATH='svn+ssh://repos.archlinux.org/srv/repos/svn-packages/svn' -msg "Work will be done in $(pwd)/rebuilds" +msg "Work will be done in %s" "$(pwd)/rebuilds" REBUILD_ROOT="$(pwd)/rebuilds" mkdir -p "$REBUILD_ROOT" @@ -63,11 +63,11 @@ FAILED="" for pkg in $pkgs; do cd "$REBUILD_ROOT/svn-packages" - msg2 "Building '$pkg'" + msg2 "Building '%s'" "$pkg" /usr/bin/svn update "$pkg" if [[ ! -d "$pkg/trunk" ]]; then FAILED="$FAILED $pkg" - warning "$pkg does not exist in SVN" + warning "%s does not exist in SVN" "$pkg" continue fi cd "$pkg/trunk/" @@ -76,14 +76,14 @@ for pkg in $pkgs; do if ! sudo makechrootpkg -u -d -r "$chrootdir" -- --noconfirm; then FAILED="$FAILED $pkg" - error "$pkg Failed!" + error "%s Failed!" "$pkg" else pkgfile=$(pkg_from_pkgbuild) if [[ -e $pkgfile ]]; then - msg2 "$pkg Complete" + msg2 "%s Complete" "$pkg" else FAILED="$FAILED $pkg" - error "$pkg Failed, no package built!" + error "%s Failed, no package built!" "$pkg" fi fi done @@ -92,7 +92,7 @@ cd "$REBUILD_ROOT" if [[ -n $FAILED ]]; then msg 'Packages failed:' for pkg in $FAILED; do - msg2 "$pkg" + msg2 "%s" "$pkg" done fi -- 2.12.1
From: Luke Shumaker <lukeshu@parabola.nu> It was displaing the value of the `makepkg_args` variable, which may have already been changed by the argument parsing by the time it gets to `-h`. Now there is a separate `default_makepkg_args` variable. --- makechrootpkg.in | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 5329f76..31d57d3 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -15,7 +15,8 @@ m4_include(lib/archroot.sh) shopt -s nullglob -makepkg_args=(-s --noconfirm -L --holdver) +default_makepkg_args=(-s --noconfirm -L --holdver) +makepkg_args=("${default_makepkg_args[@]}") repack=false update_first=false clean_first=false @@ -54,7 +55,7 @@ usage() { echo 'from makepkg.conf(5), if those variables are not part of the' echo 'environment.' echo '' - echo "Default makepkg args: ${makepkg_args[*]}" + echo "Default makepkg args: ${default_makepkg_args[*]}" echo '' echo 'Flags:' echo '-h This help' -- 2.12.1
From: Luke Shumaker <lukeshu@parabola.nu> --- arch-nspawn.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch-nspawn.in b/arch-nspawn.in index 7481d82..ca1fbc3 100644 --- a/arch-nspawn.in +++ b/arch-nspawn.in @@ -17,7 +17,7 @@ working_dir='' usage() { echo "Usage: ${0##*/} [options] working-dir [systemd-nspawn arguments]" - echo "A wrapper around systemd-nspawn. Provides support for pacman." + echo "A wrapper around systemd-nspawn. Provides support for pacman." echo echo ' options:' echo ' -C <file> Location of a pacman config file' @@ -87,7 +87,7 @@ umask 0022 if [[ ! -f "$working_dir/.arch-chroot" ]]; then die "'%s' does not appear to be an Arch chroot." "$working_dir" elif [[ $(cat "$working_dir/.arch-chroot") != $CHROOT_VERSION ]]; then - die "chroot '%s' is not at version %s. Please rebuild." "$working_dir" "$CHROOT_VERSION" + die "chroot '%s' is not at version %s. Please rebuild." "$working_dir" "$CHROOT_VERSION" fi build_mount_args -- 2.12.1
On Sun, Apr 02, 2017 at 04:09:57AM -0400, lukeshu@lukeshu.com wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
---
If you want to change this, I have no strong feelings one way or the other, but this is only a small part of what would need to be done. See the usage messages makechrootpkg, mkarchroot, the template pacman.conf and makepkg.conf files, some other log lines in makechrootpkg....
arch-nspawn.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch-nspawn.in b/arch-nspawn.in index 7481d82..ca1fbc3 100644 --- a/arch-nspawn.in +++ b/arch-nspawn.in @@ -17,7 +17,7 @@ working_dir=''
usage() { echo "Usage: ${0##*/} [options] working-dir [systemd-nspawn arguments]" - echo "A wrapper around systemd-nspawn. Provides support for pacman." + echo "A wrapper around systemd-nspawn. Provides support for pacman." echo echo ' options:' echo ' -C <file> Location of a pacman config file' @@ -87,7 +87,7 @@ umask 0022 if [[ ! -f "$working_dir/.arch-chroot" ]]; then die "'%s' does not appear to be an Arch chroot." "$working_dir" elif [[ $(cat "$working_dir/.arch-chroot") != $CHROOT_VERSION ]]; then - die "chroot '%s' is not at version %s. Please rebuild." "$working_dir" "$CHROOT_VERSION" + die "chroot '%s' is not at version %s. Please rebuild." "$working_dir" "$CHROOT_VERSION" fi
build_mount_args -- 2.12.1
On Sun, Apr 2, 2017 at 2:08 PM Dave Reisner <d@falconindy.com> wrote:
On Sun, Apr 02, 2017 at 04:09:57AM -0400, lukeshu@lukeshu.com wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
---
If you want to change this, I have no strong feelings one way or the other, but this is only a small part of what would need to be done. See the usage messages makechrootpkg, mkarchroot, the template pacman.conf and makepkg.conf files, some other log lines in makechrootpkg....
Let's keep one space. This is a style "rule" I disagree with.
On Sun, 02 Apr 2017 09:11:03 -0400, Jan Alexander Steffens via arch-projects wrote:
On Sun, Apr 2, 2017 at 2:08 PM Dave Reisner <d@falconindy.com> wrote:
On Sun, Apr 02, 2017 at 04:09:57AM -0400, lukeshu@lukeshu.com wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
---
If you want to change this, I have no strong feelings one way or the other, but this is only a small part of what would need to be done. See the usage messages makechrootpkg, mkarchroot, the template pacman.conf and makepkg.conf files, some other log lines in makechrootpkg....
Let's keep one space. This is a style "rule" I disagree with.
Ok. I have no problem if you don't want to pick this patch. -- Happy hacking, ~ Luke Shumaker
From: Luke Shumaker <lukeshu@parabola.nu> --- makechrootpkg.in | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 31d57d3..9566b2e 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -190,9 +190,7 @@ EOF { printf '#!/bin/bash\n' declare -f _chrootbuild - printf '_chrootbuild' - printf ' %q' "${makepkg_args[@]}" - printf ' || exit\n' + printf '_chrootbuild "$@" || exit\n' if $run_namcap; then declare -f _chrootnamcap @@ -338,7 +336,7 @@ if arch-nspawn "$copydir" \ --bind="$PWD:/startdir" \ --bind="$SRCDEST:/srcdest" \ "${bindmounts_ro[@]}" "${bindmounts_rw[@]}" \ - /chrootbuild + /chrootbuild "${makepkg_args[@]}" then move_products else -- 2.12.1
From: Luke Shumaker <lukeshu@parabola.nu> --- makechrootpkg.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 9566b2e..774ebcf 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -231,7 +231,7 @@ download_sources() { (( $? != 0 )) && die "Could not download sources." # Clean up garbage from verifysource - rm -rf $builddir + rm -rf "$builddir" } move_products() { -- 2.12.1
From: Luke Shumaker <lukeshu@parabola.nu> --- makechrootpkg.in | 1 - 1 file changed, 1 deletion(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 774ebcf..0650f2d 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -20,7 +20,6 @@ makepkg_args=("${default_makepkg_args[@]}") repack=false update_first=false clean_first=false -install_pkg= run_namcap=false temp_chroot=false chrootdir= -- 2.12.1
From: Luke Shumaker <lukeshu@parabola.nu> --- lib/common.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/common.sh b/lib/common.sh index 19aa7de..4536668 100644 --- a/lib/common.sh +++ b/lib/common.sh @@ -1,4 +1,5 @@ #!/hint/bash +# This may be included with or without `set -euE` # License: Unspecified @@ -8,7 +9,7 @@ export LANG=C shopt -s extglob # check if messages are to be printed using color -unset ALL_OFF BOLD BLUE GREEN RED YELLOW +declare ALL_OFF='' BOLD='' BLUE='' GREEN='' RED='' YELLOW='' if [[ -t 2 ]]; then # prefer terminal safe colored and bold text when tput is supported if tput setaf 0 &>/dev/null; then @@ -65,14 +66,14 @@ stat_done() { _setup_workdir=false setup_workdir() { - [[ -z $WORKDIR ]] && WORKDIR=$(mktemp -d --tmpdir "${0##*/}.XXXXXXXXXX") + [[ -z ${WORKDIR:-} ]] && WORKDIR=$(mktemp -d --tmpdir "${0##*/}.XXXXXXXXXX") _setup_workdir=true trap 'trap_abort' INT QUIT TERM HUP trap 'trap_exit' EXIT } cleanup() { - if [[ -n $WORKDIR ]] && $_setup_workdir; then + if [[ -n ${WORKDIR:-} ]] && $_setup_workdir; then rm -rf "$WORKDIR" fi exit ${1:-0} -- 2.12.1
From: Luke Shumaker <lukeshu@parabola.nu> This is similar to common C #ifdef guards. I was tempted to wrap the entire thing in the if/fi, rather than use 'return' to bail early. However, that means it won't execute anything until after it reaches 'fi'. And if `shopt -s extglob` isn't executed before parsing, then it will syntax-error on the extended globs. One solution would have been to move `shopt -s extglob` up above the include-guard. But the committed solution is all-around simpler. --- lib/common.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/common.sh b/lib/common.sh index 4536668..c9afc36 100644 --- a/lib/common.sh +++ b/lib/common.sh @@ -3,6 +3,9 @@ # License: Unspecified +[[ -z ${_INCLUDE_COMMON_SH:-} ]] || return 0 +_INCLUDE_COMMON_SH=true + # Avoid any encoding problems export LANG=C -- 2.12.1
participants (4)
-
Dave Reisner
-
Jan Alexander Steffens
-
Luke Shumaker
-
lukeshu@lukeshu.com