[arch-projects] [dbscripts] [PATCH v2 0/8] PKGEXT fixup
From: Luke Shumaker <lukeshu@parabola.nu> This incorporates and improves on work from 3 previously submitted patch sets: 1. My testcase patch, but - take Eli's suggestion to simplify db-update.bats - add another commit so we don't hit a bug in BATS 2. Renaming PKGEXT->PKGEXT_glob from my first fix patchset 3. Eli's fix/extglob patchset, but - re-order things between commits - also accept ".tar" (with no compression suffix) - use `old_pkgs=()` instead of `unset old_pkgs` - replace `[ -ge 1]` with `(( > 0 ))` instead of `(( > 1 ))` - db-update: use `readarray < <(... | sort -u)` instead of the O(n^2) op of using in_array in a for loop and appending to the array - common.bash, sourceballs.bats: update to work with shopt -s extglob nullglob globstar; the tests needed updated too, not just the main code The last 3 commits aren't really related to the goal of the patchset, but I wanted to include all of the work from Eli's fix patchset. I don't mean to take credit away from Eli by re-working his patches (I credit him in the commit messages); I just wanted to make it clearer what is accomplished by each change, and how each of the changes relate to our goals; as well as actually testing each of them against the test suite. Luke Shumaker (8): test: common.bash:__getCheckSum: Don't rely on IFS test: db-update: @test "update same any package to same repository fails": change PKGEXT config: Rename PKGEXT to PKGEXT_glob Correctly treat PKGEXT_glob as a glob config: let PKGEXT_glob be an extglob; have its value match makepkg ftpdir-cleanup: fix typo in a comment ("pacakge") Replace all instances of `find` command with bash globbing ftpdir-cleanup, sourceballs: swap out [ -ge 1 ] for (( > 0 )) config | 4 +++- cron-jobs/ftpdir-cleanup | 24 ++++++++++++++++++------ cron-jobs/sourceballs | 20 +++++++++++++++----- db-functions | 12 ++++++++++-- db-move | 4 ++-- db-update | 16 ++++++++++------ test/cases/db-repo-add.bats | 6 +++--- test/cases/db-update.bats | 5 +++-- test/cases/ftpdir-cleanup.bats | 4 ++-- test/cases/sourceballs.bats | 4 ++-- test/lib/common.bash | 21 ++++++++++++++++----- 11 files changed, 84 insertions(+), 36 deletions(-) -- Happy hacking, ~ Luke Shumaker
From: Luke Shumaker <lukeshu@parabola.nu> I managed to stumble across a bug in BATS where the run() function screwed with the global IFS. This breaks __getCheckSum(). The bug has been fixed in git, but isn't in a release yet; go ahead and work around it. The code getting more robust isn't a bad thing! https://github.com/sstephenson/bats/issues/89 --- test/lib/common.bash | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/lib/common.bash b/test/lib/common.bash index 540e403..cad4e13 100644 --- a/test/lib/common.bash +++ b/test/lib/common.bash @@ -9,8 +9,9 @@ __updatePKGBUILD() { } __getCheckSum() { - local result=($(sha1sum $1)) - echo ${result[0]} + local result + result="$(sha1sum "$1")" + echo "${result%% *}" } __buildPackage() { -- 2.16.1
From: Luke Shumaker <lukeshu@parabola.nu> This has the test change PKGEXT the second time it tries to release the package. Currently, this causes the tests to fail. That's a good thing; it's checking for the regression where db-functions:check_pkgrepos isn't treating PKGEXT as a glob. Without this, that regression didn't cause test failure because the checks right after it were tripping anyway. https://lists.archlinux.org/pipermail/arch-projects/2018-February/004742.htm... --- test/cases/db-update.bats | 3 ++- test/lib/common.bash | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/test/cases/db-update.bats b/test/cases/db-update.bats index 1da7eef..36511c9 100644 --- a/test/cases/db-update.bats +++ b/test/cases/db-update.bats @@ -92,8 +92,9 @@ load ../lib/common db-update checkPackage extra pkg-any-a - releasePackage extra pkg-any-a + PKGEXT=.pkg.tar.gz releasePackage extra pkg-any-a run db-update + [[ -z $BUILDDIR ]] || rm -f "${BUILDDIR}/$(__getCheckSum "${TMP}/svn-packages-copy/pkg-any-a/trunk/PKGBUILD")"/*.pkg.tar.gz{,.sig} [ "$status" -ne 0 ] } diff --git a/test/lib/common.bash b/test/lib/common.bash index cad4e13..d34af8a 100644 --- a/test/lib/common.bash +++ b/test/lib/common.bash @@ -14,6 +14,11 @@ __getCheckSum() { echo "${result%% *}" } +# Check if a file exists, even if the file uses wildcards +__isGlobfile() { + [[ -f $1 ]] +} + __buildPackage() { local pkgdest=${1:-.} local p @@ -24,7 +29,7 @@ __buildPackage() { if [[ -n ${BUILDDIR} ]]; then cache=${BUILDDIR}/$(__getCheckSum PKGBUILD) - if [[ -d ${cache} ]]; then + if __isGlobfile "${cache}"/*${PKGEXT}; then cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} return 0 else -- 2.16.1
On Sun, Feb 18, 2018 at 12:17:30PM -0500, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
This has the test change PKGEXT the second time it tries to release the package. Currently, this causes the tests to fail. That's a good thing; it's checking for the regression where db-functions:check_pkgrepos isn't treating PKGEXT as a glob.
Without this, that regression didn't cause test failure because the checks right after it were tripping anyway.
https://lists.archlinux.org/pipermail/arch-projects/2018-February/004742.htm... --- test/cases/db-update.bats | 3 ++- test/lib/common.bash | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/test/cases/db-update.bats b/test/cases/db-update.bats index 1da7eef..36511c9 100644 --- a/test/cases/db-update.bats +++ b/test/cases/db-update.bats @@ -92,8 +92,9 @@ load ../lib/common db-update checkPackage extra pkg-any-a
- releasePackage extra pkg-any-a + PKGEXT=.pkg.tar.gz releasePackage extra pkg-any-a run db-update + [[ -z $BUILDDIR ]] || rm -f "${BUILDDIR}/$(__getCheckSum "${TMP}/svn-packages-copy/pkg-any-a/trunk/PKGBUILD")"/*.pkg.tar.gz{,.sig} [ "$status" -ne 0 ] }
diff --git a/test/lib/common.bash b/test/lib/common.bash index cad4e13..d34af8a 100644 --- a/test/lib/common.bash +++ b/test/lib/common.bash @@ -14,6 +14,11 @@ __getCheckSum() { echo "${result%% *}" }
+# Check if a file exists, even if the file uses wildcards +__isGlobfile() {
Frankly, this function name and comment sucks, because it says nothing about quoting. As I read the comment, I'm lead to believe that given a file "foobar" existing, I can call: __isGlobfile "foo*", and this will succeed. To the naive reader, you might even believe this claim based on the unquotedness of $1 within the -f test.
+ [[ -f $1 ]] +}
+ __buildPackage() { local pkgdest=${1:-.} local p @@ -24,7 +29,7 @@ __buildPackage() {
if [[ -n ${BUILDDIR} ]]; then cache=${BUILDDIR}/$(__getCheckSum PKGBUILD) - if [[ -d ${cache} ]]; then + if __isGlobfile "${cache}"/*${PKGEXT}; then cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} return 0 else -- 2.16.1
From: Luke Shumaker <lukeshu@parabola.nu> Unlike the other *EXT variables, which are prescriptive, PKGEXT is descriptive, and is a glob (this has always been the case). This is confusing because of the other variables, and because it is used prescriptively in makepkg.conf. Simply put, the configuration variable name PKGEXT is overloaded. Let's rename the glob version, to make things clearer. Now, in test/lib/common.bash, there *are* 2 places where it is used prescriptively. How does that work!? The value has a glob character in it! Well, because of sloppy quoting, it just kind of works out. So, in those places, *don't* rename it to PKGEXT_glob, but introduce a local PKGEXT variable with a prescriptive value. In db-update.bats, there's a place where it was set to .pkg.tar.gz both descriptively and prescriptively; so set both PKGEXT and PKGEXT_glob in that case. --- config | 4 +++- cron-jobs/ftpdir-cleanup | 6 +++--- db-functions | 4 ++-- db-move | 4 ++-- db-update | 8 ++++---- test/cases/db-repo-add.bats | 6 +++--- test/cases/db-update.bats | 4 ++-- test/cases/ftpdir-cleanup.bats | 4 ++-- test/lib/common.bash | 8 +++++--- 9 files changed, 26 insertions(+), 22 deletions(-) diff --git a/config b/config index d2c1942..13fe202 100644 --- a/config +++ b/config @@ -23,10 +23,12 @@ LOCK_TIMEOUT=300 STAGING="$HOME/staging" TMPDIR="/var/tmp" ARCHES=(x86_64) +# prescriptive DBEXT=".db.tar.gz" FILESEXT=".files.tar.gz" -PKGEXT=".pkg.tar.?z" SRCEXT=".src.tar.gz" +# descriptive; bash glob listing allowed extensions. +PKGEXT_glob=".pkg.tar.?z" # Allowed licenses: get sourceballs only for licenses in this array ALLOWED_LICENSES=('GPL' 'GPL1' 'GPL2' 'GPL3' 'LGPL' 'LGPL1' 'LGPL2' 'LGPL2.1' 'LGPL3') diff --git a/cron-jobs/ftpdir-cleanup b/cron-jobs/ftpdir-cleanup index 2f3d5aa..4dc02a0 100755 --- a/cron-jobs/ftpdir-cleanup +++ b/cron-jobs/ftpdir-cleanup @@ -38,7 +38,7 @@ for repo in ${PKGREPOS[@]}; do continue fi # get a list of actual available package files - find "${FTP_BASE}/${repo}/os/${arch}" -xtype f -name "*${PKGEXT}" -printf '%f\n' | sort > "${WORKDIR}/repo-${repo}-${arch}" + find "${FTP_BASE}/${repo}/os/${arch}" -xtype f -name "*${PKGEXT_glob}" -printf '%f\n' | sort > "${WORKDIR}/repo-${repo}-${arch}" # get a list of package files defined in the repo db bsdtar -xOf "${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT}" | awk '/^%FILENAME%/{getline;print}' | sort > "${WORKDIR}/db-${repo}-${arch}" @@ -62,7 +62,7 @@ for repo in ${PKGREPOS[@]}; do done # get a list of all available packages in the pacakge pool -find "$FTP_BASE/${PKGPOOL}" -name "*${PKGEXT}" -printf '%f\n' | sort > "${WORKDIR}/pool" +find "$FTP_BASE/${PKGPOOL}" -name "*${PKGEXT_glob}" -printf '%f\n' | sort > "${WORKDIR}/pool" # create a list of packages in our db find "${WORKDIR}" -maxdepth 1 -type f -name 'db-*' -exec cat {} \; | sort -u > "${WORKDIR}/db" @@ -75,7 +75,7 @@ if [ ${#old_pkgs[@]} -ge 1 ]; then done fi -old_pkgs=($(find ${CLEANUP_DESTDIR} -type f -name "*${PKGEXT}" -mtime +${CLEANUP_KEEP} -printf '%f\n')) +old_pkgs=($(find ${CLEANUP_DESTDIR} -type f -name "*${PKGEXT_glob}" -mtime +${CLEANUP_KEEP} -printf '%f\n')) if [ ${#old_pkgs[@]} -ge 1 ]; then msg "Removing old packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do diff --git a/db-functions b/db-functions index e8949d7..84be241 100644 --- a/db-functions +++ b/db-functions @@ -374,8 +374,8 @@ check_pkgrepos() { local pkgver="$(getpkgver ${pkgfile})" || return 1 local pkgarch="$(getpkgarch ${pkgfile})" || return 1 - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXT} ]] && return 1 - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXT}.sig ]] && return 1 + [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXT_glob} ]] && return 1 + [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXT_glob}.sig ]] && return 1 [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgfile##*/} ]] && return 1 [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgfile##*/}.sig ]] && return 1 diff --git a/db-move b/db-move index 37a9884..e3bc16e 100755 --- a/db-move +++ b/db-move @@ -49,7 +49,7 @@ for pkgbase in ${args[@]:2}; do for pkgname in ${pkgnames[@]}; do for tarch in ${tarches[@]}; do - getpkgfile "${ftppath_from}/${tarch}/"${pkgname}-${pkgver}-${pkgarch}${PKGEXT} >/dev/null + getpkgfile "${ftppath_from}/${tarch}/"${pkgname}-${pkgver}-${pkgarch}${PKGEXT_glob} >/dev/null done done continue 2 @@ -95,7 +95,7 @@ for pkgbase in ${args[@]:2}; do for pkgname in ${pkgnames[@]}; do for tarch in ${tarches[@]}; do - pkgpath=$(getpkgfile "${ftppath_from}/${tarch}/"${pkgname}-${pkgver}-${pkgarch}${PKGEXT}) + pkgpath=$(getpkgfile "${ftppath_from}/${tarch}/"${pkgname}-${pkgver}-${pkgarch}${PKGEXT_glob}) pkgfile="${pkgpath##*/}" ln -s "../../../${PKGPOOL}/${pkgfile}" ${ftppath_to}/${tarch}/ diff --git a/db-update b/db-update index 45755a4..4afeb6e 100755 --- a/db-update +++ b/db-update @@ -9,7 +9,7 @@ if (( $# >= 1 )); then fi # Find repos with packages to release -if ! staging_repos=($(find "${STAGING}" -mindepth 1 -type f -name "*${PKGEXT}" -printf '%h\n' | sort -u)); then +if ! staging_repos=($(find "${STAGING}" -mindepth 1 -type f -name "*${PKGEXT_glob}" -printf '%h\n' | sort -u)); then die "Could not read %s" "$STAGING" fi @@ -32,7 +32,7 @@ for repo in ${repos[@]}; do if ! check_repo_permission "${repo}"; then die "You don't have permission to update packages in %s" "$repo" fi - pkgs=($(getpkgfiles "${STAGING}/${repo}/"*${PKGEXT})) + pkgs=($(getpkgfiles "${STAGING}/${repo}/"*${PKGEXT_glob})) if (( $? == 0 )); then for pkg in ${pkgs[@]}; do if [[ -h ${pkg} ]]; then @@ -70,10 +70,10 @@ done for repo in ${repos[@]}; do msg "Updating [%s]..." "$repo" - any_pkgs=($(getpkgfiles "${STAGING}/${repo}/"*-any${PKGEXT} 2>/dev/null)) + any_pkgs=($(getpkgfiles "${STAGING}/${repo}/"*-any${PKGEXT_glob} 2>/dev/null)) for pkgarch in ${ARCHES[@]}; do add_pkgs=() - arch_pkgs=($(getpkgfiles "${STAGING}/${repo}/"*-${pkgarch}${PKGEXT} 2>/dev/null)) + arch_pkgs=($(getpkgfiles "${STAGING}/${repo}/"*-${pkgarch}${PKGEXT_glob} 2>/dev/null)) for pkg in ${arch_pkgs[@]} ${any_pkgs[@]}; do pkgfile="${pkg##*/}" msg2 "${pkgfile} (${pkgarch})" diff --git a/test/cases/db-repo-add.bats b/test/cases/db-repo-add.bats index 9fde381..ace7452 100644 --- a/test/cases/db-repo-add.bats +++ b/test/cases/db-repo-add.bats @@ -14,10 +14,10 @@ __movePackageToRepo() { fi # FIXME: pkgbase might not be part of the package filename - mv -v "${STAGING}"/${repo}/${pkgbase}-*-*-${arch}${PKGEXT}{,.sig} "${FTP_BASE}/${PKGPOOL}/" + mv -v "${STAGING}"/${repo}/${pkgbase}-*-*-${arch}${PKGEXT_glob}{,.sig} "${FTP_BASE}/${PKGPOOL}/" for tarch in ${tarches[@]}; do - ln -sv ${FTP_BASE}/${PKGPOOL}/${pkgbase}-*-*-${arch}${PKGEXT} "${FTP_BASE}/${repo}/os/${tarch}/" - ln -sv ${FTP_BASE}/${PKGPOOL}/${pkgbase}-*-*-${arch}${PKGEXT}.sig "${FTP_BASE}/${repo}/os/${tarch}/" + ln -sv ${FTP_BASE}/${PKGPOOL}/${pkgbase}-*-*-${arch}${PKGEXT_glob} "${FTP_BASE}/${repo}/os/${tarch}/" + ln -sv ${FTP_BASE}/${PKGPOOL}/${pkgbase}-*-*-${arch}${PKGEXT_glob}.sig "${FTP_BASE}/${repo}/os/${tarch}/" done } diff --git a/test/cases/db-update.bats b/test/cases/db-update.bats index 36511c9..c1b8eb4 100644 --- a/test/cases/db-update.bats +++ b/test/cases/db-update.bats @@ -92,7 +92,7 @@ load ../lib/common db-update checkPackage extra pkg-any-a - PKGEXT=.pkg.tar.gz releasePackage extra pkg-any-a + PKGEXT=.pkg.tar.gz PKGEXT_glob=.pkg.tar.gz releasePackage extra pkg-any-a run db-update [[ -z $BUILDDIR ]] || rm -f "${BUILDDIR}/$(__getCheckSum "${TMP}/svn-packages-copy/pkg-any-a/trunk/PKGBUILD")"/*.pkg.tar.gz{,.sig} [ "$status" -ne 0 ] @@ -151,7 +151,7 @@ load ../lib/common @test "add invalid signed package fails" { local p releasePackage extra 'pkg-any-a' - for p in "${STAGING}"/extra/*${PKGEXT}; do + for p in "${STAGING}"/extra/*${PKGEXT_glob}; do unxz $p xz -0 ${p%%.xz} done diff --git a/test/cases/ftpdir-cleanup.bats b/test/cases/ftpdir-cleanup.bats index 6280ce0..7dfad4a 100644 --- a/test/cases/ftpdir-cleanup.bats +++ b/test/cases/ftpdir-cleanup.bats @@ -13,8 +13,8 @@ __checkRepoRemovedPackage() { local pkgname for pkgname in $(__getPackageNamesFromPackageBase ${pkgbase}); do - [[ ! -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-*${PKGEXT} ]] - [[ ! -f ${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}-*${PKGEXT} ]] + [[ ! -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-*${PKGEXT_glob} ]] + [[ ! -f ${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}-*${PKGEXT_glob} ]] done } diff --git a/test/lib/common.bash b/test/lib/common.bash index d34af8a..94fedfe 100644 --- a/test/lib/common.bash +++ b/test/lib/common.bash @@ -26,11 +26,12 @@ __buildPackage() { local pkgarches local tarch local pkgnames + local PKGEXT="${PKGEXT:-.pkg.tar.xz}" if [[ -n ${BUILDDIR} ]]; then cache=${BUILDDIR}/$(__getCheckSum PKGBUILD) - if __isGlobfile "${cache}"/*${PKGEXT}; then - cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} + if __isGlobfile "${cache}"/*${PKGEXT_glob}; then + cp -Lv ${cache}/*${PKGEXT_glob}{,.sig} ${pkgdest} return 0 else mkdir -p ${cache} @@ -174,6 +175,7 @@ checkPackageDB() { local repoarches local pkgfile local pkgname + local PKGEXT="${PKGEXT:-.pkg.tar.xz}" # FIXME: We guess the location of the PKGBUILD used for this repo # We cannot read from trunk as __updatePKGBUILD() might have bumped the version @@ -221,7 +223,7 @@ checkPackageDB() { for db in ${DBEXT} ${FILESEXT}; do [ -r "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" ] - bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep -q "${pkgfile%${PKGEXT}}" + bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep -q "${pkgfile%${PKGEXT_glob}}" done done done -- 2.16.1
From: Luke Shumaker <lukeshu@parabola.nu> Several places treated PKGEXT as a fixed string in [[ -f ]] existence checks. Fix that elegantly by introducing an is_globfile function. This fixes the failing db-update.bats "update same any package to same repository fails" test. This is based on a patch by Eli Schwartz <eschwartz@archlinux.org> --- db-functions | 9 +++++++-- test/cases/ftpdir-cleanup.bats | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/db-functions b/db-functions index 84be241..769d7ef 100644 --- a/db-functions +++ b/db-functions @@ -16,6 +16,11 @@ restore_umask () { umask $UMASK >/dev/null } +# Check if a file exists, even if the file uses wildcards +is_globfile() { + [[ -f $1 ]] +} + # just like mv -f, but we touch the file and then copy the content so # default ACLs in the target dir will be applied mv_acl() { @@ -374,8 +379,8 @@ check_pkgrepos() { local pkgver="$(getpkgver ${pkgfile})" || return 1 local pkgarch="$(getpkgarch ${pkgfile})" || return 1 - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXT_glob} ]] && return 1 - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXT_glob}.sig ]] && return 1 + is_globfile "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXT_glob} && return 1 + is_globfile "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXT_glob}.sig && return 1 [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgfile##*/} ]] && return 1 [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgfile##*/}.sig ]] && return 1 diff --git a/test/cases/ftpdir-cleanup.bats b/test/cases/ftpdir-cleanup.bats index 7dfad4a..efc18a8 100644 --- a/test/cases/ftpdir-cleanup.bats +++ b/test/cases/ftpdir-cleanup.bats @@ -13,8 +13,8 @@ __checkRepoRemovedPackage() { local pkgname for pkgname in $(__getPackageNamesFromPackageBase ${pkgbase}); do - [[ ! -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-*${PKGEXT_glob} ]] - [[ ! -f ${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}-*${PKGEXT_glob} ]] + ! is_globfile "${FTP_BASE}/${PKGPOOL}/${pkgname}"-*${PKGEXT_glob} + ! is_globfile "${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}"-*${PKGEXT_glob} done } -- 2.16.1
On Sun, 18 Feb 2018 12:17:32 -0500, Luke Shumaker wrote:
diff --git a/test/cases/ftpdir-cleanup.bats b/test/cases/ftpdir-cleanup.bats index 7dfad4a..efc18a8 100644 --- a/test/cases/ftpdir-cleanup.bats +++ b/test/cases/ftpdir-cleanup.bats @@ -13,8 +13,8 @@ __checkRepoRemovedPackage() { local pkgname
for pkgname in $(__getPackageNamesFromPackageBase ${pkgbase}); do - [[ ! -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-*${PKGEXT_glob} ]] - [[ ! -f ${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}-*${PKGEXT_glob} ]] + ! is_globfile "${FTP_BASE}/${PKGPOOL}/${pkgname}"-*${PKGEXT_glob} + ! is_globfile "${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}"-*${PKGEXT_glob} done
Shoot, that needs to be __isGlobfile; we don't load db-functions here; is_globfile is undefined. It's basically doing "! false". No mater how many times you look over it before hitting send... -- Happy hacking, ~ Luke Shumaker
From: Luke Shumaker <lukeshu@parabola.nu> 1. In order to support PKGEXT_glob being an extglob, we must switch several uses of `find` (in db-update and ftpdir-cleanup) to use bash globbing instead, as `find` can't be made to use extended globbing. In general, this technique requires us to also enable nullglob (which we do globally in db-functions). Of the 4 searches that we have to rewrite: - (ftpdir-cleanup) Two were just being used to strip leading directory paths, and can be replaced by ${filepath##*/} in a for loop. - (ftpdir-cleanup) One was checking the modification time of the files, and can be replaced with touch(1) and [[ -nt ]]. Although this introduces an additional temporary file, this is not such a big deal. - (db-update) One was getting leading directory paths, this can be replaced with globstar (which we also enable globally in db-functions) and ${filepath%/*} in a for loop. We can drop the error handling that was on that search, as the old code never aborted on errors anyway, as without `set -o pipefail` the sort command swallowed the return code. 2. In addition to enabling exglob in db-functions for the main code, we also need to do it in test/lib/common.bash for the tests. Go ahead and also enable globstar and nullglob there too, for consistency with db-functions. Turning on nullglob there in turn forces us to fix up a couple of [ -f ] glob checks in sourceballs.bats. 3. Use this new extended globbing capability to set the default value of PKGEXT_glob to reflect the behavior of makepkg (v5.0.2); the old value of PKGEXT_glob both accepted things that makepkg would reject, and rejected things that makepkg would accept. This is based on patches by Eli Schwartz <eschwartz@archlinux.org> --- config | 4 ++-- cron-jobs/ftpdir-cleanup | 18 +++++++++++++++--- db-functions | 3 +++ db-update | 10 +++++++--- test/cases/sourceballs.bats | 4 ++-- test/lib/common.bash | 3 +++ 6 files changed, 32 insertions(+), 10 deletions(-) diff --git a/config b/config index 13fe202..d2d92ba 100644 --- a/config +++ b/config @@ -27,8 +27,8 @@ ARCHES=(x86_64) DBEXT=".db.tar.gz" FILESEXT=".files.tar.gz" SRCEXT=".src.tar.gz" -# descriptive; bash glob listing allowed extensions. -PKGEXT_glob=".pkg.tar.?z" +# descriptive; bash glob listing allowed extensions. Note that db-functions turns on extglob. +PKGEXT_glob=".pkg.tar?(.gz|.bz2|.xz|.lrz|.lzo|.Z)" # Allowed licenses: get sourceballs only for licenses in this array ALLOWED_LICENSES=('GPL' 'GPL1' 'GPL2' 'GPL3' 'LGPL' 'LGPL1' 'LGPL2' 'LGPL2.1' 'LGPL3') diff --git a/cron-jobs/ftpdir-cleanup b/cron-jobs/ftpdir-cleanup index 4dc02a0..20d579f 100755 --- a/cron-jobs/ftpdir-cleanup +++ b/cron-jobs/ftpdir-cleanup @@ -38,7 +38,11 @@ for repo in ${PKGREPOS[@]}; do continue fi # get a list of actual available package files - find "${FTP_BASE}/${repo}/os/${arch}" -xtype f -name "*${PKGEXT_glob}" -printf '%f\n' | sort > "${WORKDIR}/repo-${repo}-${arch}" + for f in "${FTP_BASE}"/${repo}/os/${arch}/*${PKGEXT_glob}; do + if [[ -f $f ]]; then + printf '%s\n' "${f##*/}" + fi + done | sort > "${WORKDIR}/repo-${repo}-${arch}" # get a list of package files defined in the repo db bsdtar -xOf "${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT}" | awk '/^%FILENAME%/{getline;print}' | sort > "${WORKDIR}/db-${repo}-${arch}" @@ -62,7 +66,9 @@ for repo in ${PKGREPOS[@]}; do done # get a list of all available packages in the pacakge pool -find "$FTP_BASE/${PKGPOOL}" -name "*${PKGEXT_glob}" -printf '%f\n' | sort > "${WORKDIR}/pool" +for f in "$FTP_BASE/${PKGPOOL}"/*${PKGEXT_glob}; do + printf '%s\n' "${f##*/}" +done | sort > "${WORKDIR}/pool" # create a list of packages in our db find "${WORKDIR}" -maxdepth 1 -type f -name 'db-*' -exec cat {} \; | sort -u > "${WORKDIR}/db" @@ -75,7 +81,13 @@ if [ ${#old_pkgs[@]} -ge 1 ]; then done fi -old_pkgs=($(find ${CLEANUP_DESTDIR} -type f -name "*${PKGEXT_glob}" -mtime +${CLEANUP_KEEP} -printf '%f\n')) +old_pkgs=() +touch -d "${CLEANUP_KEEP} days ago" "${WORKDIR}/cleanup_timestamp" +for f in "${CLEANUP_DESTDIR}"/**/*${PKGEXT_glob}; do + if [[ ${WORKDIR}/cleanup_timestamp -nt $f ]]; then + old_pkgs+=("${f##*/}") + fi +done if [ ${#old_pkgs[@]} -ge 1 ]; then msg "Removing old packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do diff --git a/db-functions b/db-functions index 769d7ef..8ca9205 100644 --- a/db-functions +++ b/db-functions @@ -2,6 +2,9 @@ . /usr/share/makepkg/util.sh +# global shell options for enhanced bash scripting +shopt -s extglob globstar nullglob + # Some PKGBUILDs need CARCH to be set CARCH="x86_64" diff --git a/db-update b/db-update index 4afeb6e..37cffbf 100755 --- a/db-update +++ b/db-update @@ -9,9 +9,13 @@ if (( $# >= 1 )); then fi # Find repos with packages to release -if ! staging_repos=($(find "${STAGING}" -mindepth 1 -type f -name "*${PKGEXT_glob}" -printf '%h\n' | sort -u)); then - die "Could not read %s" "$STAGING" -fi +readarray -t staging_repos < <( + for f in "${STAGING}"/**/*${PKGEXT}; do + if [[ -f $f ]]; then + printf '%s\n' "${f%/*}" + fi + done | sort -u +) repos=() for staging_repo in ${staging_repos[@]##*/}; do diff --git a/test/cases/sourceballs.bats b/test/cases/sourceballs.bats index a0a2999..df7ddd4 100644 --- a/test/cases/sourceballs.bats +++ b/test/cases/sourceballs.bats @@ -2,12 +2,12 @@ load ../lib/common __checkSourcePackage() { local pkgbase=$1 - [ -r ${FTP_BASE}/${SRCPOOL}/${pkgbase}-*${SRCEXT} ] + __isGlobfile "${FTP_BASE}/${SRCPOOL}/${pkgbase}"-*"${SRCEXT}" } __checkRemovedSourcePackage() { local pkgbase=$1 - [ ! -r ${FTP_BASE}/${SRCPOOL}/${pkgbase}-*${SRCEXT} ] + ! __isGlobfile "${FTP_BASE}/${SRCPOOL}/${pkgbase}"-*"${SRCEXT}" } @test "create simple package sourceballs" { diff --git a/test/lib/common.bash b/test/lib/common.bash index 94fedfe..9d6edeb 100644 --- a/test/lib/common.bash +++ b/test/lib/common.bash @@ -1,5 +1,8 @@ . /usr/share/makepkg/util.sh +# global shell options for enhanced bash scripting +shopt -s extglob globstar nullglob + __updatePKGBUILD() { local pkgrel -- 2.16.1
From: Luke Shumaker <lukeshu@parabola.nu> This is based on a patch by Eli Schwartz <eschwartz@archlinux.org> --- cron-jobs/ftpdir-cleanup | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cron-jobs/ftpdir-cleanup b/cron-jobs/ftpdir-cleanup index 20d579f..64d50ef 100755 --- a/cron-jobs/ftpdir-cleanup +++ b/cron-jobs/ftpdir-cleanup @@ -65,7 +65,7 @@ for repo in ${PKGREPOS[@]}; do done done -# get a list of all available packages in the pacakge pool +# get a list of all available packages in the package pool for f in "$FTP_BASE/${PKGPOOL}"/*${PKGEXT_glob}; do printf '%s\n' "${f##*/}" done | sort > "${WORKDIR}/pool" -- 2.16.1
From: Luke Shumaker <lukeshu@parabola.nu> This fully removes the use of find from the codebase, and leads to a micro-optimization in a couple cases. Of the 5 instances in these files: - (ftpdir-cleanup) One was unnecessary as `cat` can natively consume all files passed to it (from a glob) and no directory traversal was in use. We don't have to worry about the glob being empty, unless config:ARCHES or config:PKGREPOS is mis/non-configured and is empty. - (sourceballs) One was only being used to strip leading directory paths, and can be replaced by ${filepath##*/} in a for loop. - (sourceballs) Two were unnecessary as they were hardcoded to read a single file. - (sourceballs) One was checking the modification time of the files, and can be replaced with touch(1) and [[ -nt ]]. Although this introduces an additional temporary file, this is not such a big deal. This is based on patches by Eli Schwartz <eschwartz@archlinux.org> --- cron-jobs/ftpdir-cleanup | 2 +- cron-jobs/sourceballs | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/cron-jobs/ftpdir-cleanup b/cron-jobs/ftpdir-cleanup index 64d50ef..6a6c82f 100755 --- a/cron-jobs/ftpdir-cleanup +++ b/cron-jobs/ftpdir-cleanup @@ -70,7 +70,7 @@ for f in "$FTP_BASE/${PKGPOOL}"/*${PKGEXT_glob}; do printf '%s\n' "${f##*/}" done | sort > "${WORKDIR}/pool" # create a list of packages in our db -find "${WORKDIR}" -maxdepth 1 -type f -name 'db-*' -exec cat {} \; | sort -u > "${WORKDIR}/db" +cat "${WORKDIR}"/db-* 2>/dev/null | sort -u > "${WORKDIR}/db" old_pkgs=($(comm -23 "${WORKDIR}/pool" "${WORKDIR}/db")) if [ ${#old_pkgs[@]} -ge 1 ]; then diff --git a/cron-jobs/sourceballs b/cron-jobs/sourceballs index 9ab4e98..c47d5ad 100755 --- a/cron-jobs/sourceballs +++ b/cron-jobs/sourceballs @@ -46,7 +46,11 @@ for repo in ${PKGREPOS[@]}; do done # Create a list of all available source package file names -find "${FTP_BASE}/${SRCPOOL}" -xtype f -name "*${SRCEXT}" -printf '%f\n' | sort -u > "${WORKDIR}/available-src-pkgs" +for f in "${FTP_BASE}"/${SRCPOOL}/*${SRCEXT}; do + if [[ -f $f ]]; then + printf '%s\n' "${f##*/}" + fi +done | sort -u > "${WORKDIR}/available-src-pkgs" # Check for all packages if we need to build a source package for repo in ${PKGREPOS[@]}; do @@ -117,8 +121,8 @@ for repo in ${PKGREPOS[@]}; do done # Cleanup old source packages -find "${WORKDIR}" -maxdepth 1 -type f -name 'expected-src-pkgs' -exec cat {} \; | sort -u > "${WORKDIR}/expected-src-pkgs.sort" -find "${WORKDIR}" -maxdepth 1 -type f -name 'available-src-pkgs' -exec cat {} \; | sort -u > "${WORKDIR}/available-src-pkgs.sort" +sort -u "${WORKDIR}/expected-src-pkgs" > "${WORKDIR}/expected-src-pkgs.sort" +sort -u "${WORKDIR}/available-src-pkgs" > "${WORKDIR}/available-src-pkgs.sort" old_pkgs=($(comm -23 "${WORKDIR}/available-src-pkgs.sort" "${WORKDIR}/expected-src-pkgs.sort")) if [ ${#old_pkgs[@]} -ge 1 ]; then @@ -133,7 +137,13 @@ if [ ${#old_pkgs[@]} -ge 1 ]; then done fi -old_pkgs=($(find ${SOURCE_CLEANUP_DESTDIR} -type f -name "*${SRCEXT}" -mtime +${SOURCE_CLEANUP_KEEP} -printf '%f\n')) +old_pkgs=() +touch -d "${SOURCE_CLEANUP_KEEP} days ago" "${WORKDIR}/cleanup_timestamp" +for f in "${SOURCE_CLEANUP_DESTDIR}"/*${SRCEXT}; do + if [[ ${WORKDIR}/cleanup_timestamp -nt $f ]]; then + old_pkgs+=("${f##*/}") + fi +done if [ ${#old_pkgs[@]} -ge 1 ]; then msg "Removing old source packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do -- 2.16.1
From: Luke Shumaker <lukeshu@parabola.nu> This is based on a patch by Eli Schwartz <eschwartz@archlinux.org> --- cron-jobs/ftpdir-cleanup | 2 +- cron-jobs/sourceballs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cron-jobs/ftpdir-cleanup b/cron-jobs/ftpdir-cleanup index 6a6c82f..fac05bd 100755 --- a/cron-jobs/ftpdir-cleanup +++ b/cron-jobs/ftpdir-cleanup @@ -88,7 +88,7 @@ for f in "${CLEANUP_DESTDIR}"/**/*${PKGEXT_glob}; do old_pkgs+=("${f##*/}") fi done -if [ ${#old_pkgs[@]} -ge 1 ]; then +if (( ${#old_pkgs[@]} > 0 )); then msg "Removing old packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do msg2 "${old_pkg}" diff --git a/cron-jobs/sourceballs b/cron-jobs/sourceballs index c47d5ad..d8c0a43 100755 --- a/cron-jobs/sourceballs +++ b/cron-jobs/sourceballs @@ -144,7 +144,7 @@ for f in "${SOURCE_CLEANUP_DESTDIR}"/*${SRCEXT}; do old_pkgs+=("${f##*/}") fi done -if [ ${#old_pkgs[@]} -ge 1 ]; then +if (( ${#old_pkgs[@]} > 0 )); then msg "Removing old source packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do msg2 "${old_pkg}" -- 2.16.1
On 02/18/2018 12:17 PM, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
This incorporates and improves on work from 3 previously submitted patch sets:
1. My testcase patch, but - take Eli's suggestion to simplify db-update.bats - add another commit so we don't hit a bug in BATS
2. Renaming PKGEXT->PKGEXT_glob from my first fix patchset
3. Eli's fix/extglob patchset, but - re-order things between commits - also accept ".tar" (with no compression suffix) - use `old_pkgs=()` instead of `unset old_pkgs` - replace `[ -ge 1]` with `(( > 0 ))` instead of `(( > 1 ))` - db-update: use `readarray < <(... | sort -u)` instead of the O(n^2) op of using in_array in a for loop and appending to the array - common.bash, sourceballs.bats: update to work with shopt -s extglob nullglob globstar; the tests needed updated too, not just the main code
The last 3 commits aren't really related to the goal of the patchset, but I wanted to include all of the work from Eli's fix patchset.
I don't mean to take credit away from Eli by re-working his patches (I credit him in the commit messages); I just wanted to make it clearer what is accomplished by each change, and how each of the changes relate to our goals; as well as actually testing each of them against the test suite. No need to resubmit someone else's amended patches, or resubmit not-amended patches with the statement "I've tested this" (you did that for devtools). Please just leave comments on the submitted patch.
I can resubmit the patch with a "v2: fix blahblahblah" comment after the commit message detailing what changed. I just won't have time until Monday. This also makes it easier to keep track of *which* patches are rerolls of previous patches. -- Eli Schwartz Bug Wrangler and Trusted User
On Sun, 18 Feb 2018 12:29:31 -0500, Eli Schwartz wrote:
On 02/18/2018 12:17 PM, Luke Shumaker wrote:
I don't mean to take credit away from Eli by re-working his patches (I credit him in the commit messages); I just wanted to make it clearer what is accomplished by each change, and how each of the changes relate to our goals; as well as actually testing each of them against the test suite. No need to resubmit someone else's amended patches, or resubmit not-amended patches with the statement "I've tested this" (you did that for devtools). Please just leave comments on the submitted patch.
There were also rebase/merge conflicts. I wanted to have a commit that fixed glob support (is_globfile), to verify that that worked before adding extglob support; re-ordering that/splitting those commits created rebase conflicts. I verified that the test status of each of the commits is as expected. I put the PKGEXT->PKGEXT_glob commit before the fix because it turns out that it's easy to accidentally effectively revert the test during that rename; and I wanted to ensure that it was still being tested by the time the fix is applied. This, of course, meant that your patches didn't apply cleanly. Noting the 3 patchsets that this incorporates: 1. luke1: Renaming PKGEXT->PKGEXT_glob from my first fix patchset 2. luke2: My testcase patch 3. eli : Eli's fix/extglob patchset The origin of the changes in each, and it's testsuite status: # origin test subject [1/8] new PASS test: common.bash:__getCheckSum: Don't rely on IFS [2/8] luke2+reroll FAIL test: db-update: @test "update same any package to same repository fails": change PKGEXT [3/8] luke1+reroll FAIL config: Rename PKGEXT to PKGEXT_glob [4/8] eli+reroll PASS Correctly treat PKGEXT_glob as a glob [5/8] eli+reroll PASS config: let PKGEXT_glob be an extglob; have its value match makepkg [6/8] eli PASS ftpdir-cleanup: fix typo in a comment ("pacakge") [7/8] eli PASS Replace all instances of `find` command with bash globbing [8/8] eli+reroll PASS ftpdir-cleanup, sourceballs: swap out [ -ge 1 ] for (( > 0 )) I don't believe the changes that are in commits 6 or 7 changed from your work, except that they got split in to separate commits. -- Happy hacking, ~ Luke Shumaker
participants (3)
-
Dave Reisner
-
Eli Schwartz
-
Luke Shumaker