Message ID | 20180219201145.14057-2-eschwartz@archlinux.org |
---|---|
State | Accepted |
Headers | show |
Series | Fix ambiguous uses of $PKGEXT | expand |
Hi Eli, Disclaimer: the following is a bit subtle topic, so I hope it doesn't spur a lot of off-topic. On 19 February 2018 at 20:11, Eli Schwartz via arch-projects <arch-projects@archlinux.org> wrote: > Catch some cases that were missed in the previous run. > > Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> > --- > > This patch is new + refactor some changes from: > ftpdir-cleanup,sourceballs: replace external find command with bash globbing > > cron-jobs/devlist-mailer | 6 +++--- > cron-jobs/ftpdir-cleanup | 14 +++++++------- > cron-jobs/integrity-check | 2 +- > cron-jobs/sourceballs | 12 ++++++------ > cron-jobs/update-web-db | 6 +++--- > 5 files changed, 20 insertions(+), 20 deletions(-) > Is there any performance or other technical benefit to using more bashisms? Reason being, that I am slowly going through different parts of Arch making it zsh friendly. While keeping the code brief and legible, of course. Guessing that I've picked the wrong hobby? Thanks Emil
On Tue, Feb 20, 2018 at 11:59:49AM +0000, Emil Velikov via arch-projects wrote: > Hi Eli, > > Disclaimer: the following is a bit subtle topic, so I hope it doesn't > spur a lot of off-topic. > > On 19 February 2018 at 20:11, Eli Schwartz via arch-projects > <arch-projects@archlinux.org> wrote: > > Catch some cases that were missed in the previous run. > > > > Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> > > --- > > > > This patch is new + refactor some changes from: > > ftpdir-cleanup,sourceballs: replace external find command with bash globbing > > > > cron-jobs/devlist-mailer | 6 +++--- > > cron-jobs/ftpdir-cleanup | 14 +++++++------- > > cron-jobs/integrity-check | 2 +- > > cron-jobs/sourceballs | 12 ++++++------ > > cron-jobs/update-web-db | 6 +++--- > > 5 files changed, 20 insertions(+), 20 deletions(-) > > > Is there any performance or other technical benefit to using more bashisms? The scripts run under bash, so why not take advantage of bash features? For example, bash's [[ and (( are less error prone and more featureful than the POSIX [, and builtins like mapfile and read (POSIX read has an extremely limited featureset) make I/O far simpler tasks. There's plenty more to like... Please don't try to talk about performance and shell in the same sentence. These are not performance-sensitive scripts, and shell is not a language to use when performance (of almost any kind) is relevant. > Reason being, that I am slowly going through different parts of Arch > making it zsh friendly. > While keeping the code brief and legible, of course. Feel free to exemplify how conversion from bash to zsh has aided your goals while retaining portability to a supermajority of Arch systems. $ pacman -Q zsh error: package 'zsh' was not found > Guessing that I've picked the wrong hobby? Almost certainly. > Thanks > Emil
On 02/20/2018 06:59 AM, Emil Velikov wrote: > Disclaimer: the following is a bit subtle topic, so I hope it doesn't > spur a lot of off-topic. Eh, I don't mind. > Is there any performance or other technical benefit to using more bashisms? > > Reason being, that I am slowly going through different parts of Arch > making it zsh friendly. > While keeping the code brief and legible, of course. > Guessing that I've picked the wrong hobby? I think you'll probably find that few people write zsh scripts for non-interactive use. I'm not really sure what the point would be, considering it has a nonstandard syntax (bash is ubiquitous, zsh is not), and many people who would know bash would not know zsh (like me for example). AFAIK zsh should more or less run either bash or POSIX sh scripts just fine if you invoke it via a symlink named `sh` or `bash`, because zsh has a bash compatibility mode. I have no idea whether that bash compatibility mode fixes subtle things like the fact that zsh arrays are 1-indexed while bash arrays are 0-indexed, but if I had to guess, probably not. ... I can see some compelling reasons to write scripts targeting POSIX sh as a baseline, which is being *sh* friendly, not zsh friendly. But, for projects that make heavy use of bashisms anyways, I dislike using POSIX because it implies that sh will be supported in any way when it really won't be. Essentially, I prefer to go "all in". As for why you'd want them, bashisms generally look cleaner IMHO, and they add a great deal of power and flexibility to the shell. Things like [[ ... ]] are just a lot more sane in basically every way, shell arithmetic uses proper operators, etc.
On 20 February 2018 at 14:23, Eli Schwartz <eschwartz@archlinux.org> wrote: > On 02/20/2018 06:59 AM, Emil Velikov wrote: >> Disclaimer: the following is a bit subtle topic, so I hope it doesn't >> spur a lot of off-topic. > > Eh, I don't mind. > >> Is there any performance or other technical benefit to using more bashisms? >> >> Reason being, that I am slowly going through different parts of Arch >> making it zsh friendly. >> While keeping the code brief and legible, of course. >> Guessing that I've picked the wrong hobby? > > I think you'll probably find that few people write zsh scripts for > non-interactive use. I'm not really sure what the point would be, > considering it has a nonstandard syntax (bash is ubiquitous, zsh is > not), and many people who would know bash would not know zsh (like me > for example). > > AFAIK zsh should more or less run either bash or POSIX sh scripts just > fine if you invoke it via a symlink named `sh` or `bash`, because zsh > has a bash compatibility mode. I have no idea whether that bash > compatibility mode fixes subtle things like the fact that zsh arrays are > 1-indexed while bash arrays are 0-indexed, but if I had to guess, > probably not. > > ... > > I can see some compelling reasons to write scripts targeting POSIX sh as > a baseline, which is being *sh* friendly, not zsh friendly. > But, for projects that make heavy use of bashisms anyways, I dislike > using POSIX because it implies that sh will be supported in any way when > it really won't be. Essentially, I prefer to go "all in". > > As for why you'd want them, bashisms generally look cleaner IMHO, and > they add a great deal of power and flexibility to the shell. Things like > [[ ... ]] are just a lot more sane in basically every way, shell > arithmetic uses proper operators, etc. > Seems like I wasn't clear enough: The goal is not to appease zsh - but a step closer to POSIX sh friendly. I've been staring and writing bash (closer to POSIX sh really) scripts for over a decade, haven't seen what makes X cleaner over Y. Yet that's subjective, unlike the original argument - consistency rules ;-) Thanks Emil
On 02/20/2018 12:24 PM, Emil Velikov wrote: > Seems like I wasn't clear enough: > The goal is not to appease zsh - but a step closer to POSIX sh friendly. > > I've been staring and writing bash (closer to POSIX sh really) scripts > for over a decade, haven't seen what makes X cleaner over Y. > Yet that's subjective, unlike the original argument - consistency rules ;-) If you're working for "POSIX sh friendly", why are you mentioning zsh in the first place? As for targeting POSIX sh, if you can do that then sure. I have personal scripts written for sh when it makes sense (and my /bin/sh is symlinked to dash, so I actually use sh). But yeah, consistency rules.
diff --git a/cron-jobs/devlist-mailer b/cron-jobs/devlist-mailer index ca2e46b..65a5483 100755 --- a/cron-jobs/devlist-mailer +++ b/cron-jobs/devlist-mailer @@ -7,17 +7,17 @@ LIST="arch-dev-public@archlinux.org" FROM="repomaint@archlinux.org" SUBJECT="Repository Maintenance $(date +"%d-%m-%Y")" -if [ $# -ge 1 ]; then +if (( $# >= 1 )); then SUBJECT="$1 $(date +"%d-%m-%Y")" fi -if [ $# -ge 2 ]; then +if (( $# >= 2 )); then LIST="$2" fi stdin="$(cat)" #echo used to strip whitespace for checking for actual data -if [ -n "$(echo $stdin)" ]; then +if [[ -n "$(echo $stdin)" ]]; then echo "Subject: $SUBJECT To: $LIST diff --git a/cron-jobs/ftpdir-cleanup b/cron-jobs/ftpdir-cleanup index 2f3d5aa..c771950 100755 --- a/cron-jobs/ftpdir-cleanup +++ b/cron-jobs/ftpdir-cleanup @@ -9,11 +9,11 @@ clean_pkg() { if ! ${CLEANUP_DRYRUN}; then for pkg in "$@"; do - if [ -h "$pkg" ]; then + if [[ -h $pkg ]]; then rm -f "$pkg" "$pkg.sig" else mv_acl "$pkg" "$CLEANUP_DESTDIR/${pkg##*/}" - if [ -e "$pkg.sig" ]; then + if [[ -e $pkg.sig ]]; then mv_acl "$pkg.sig" "$CLEANUP_DESTDIR/${pkg##*/}.sig" fi touch "${CLEANUP_DESTDIR}/${pkg##*/}" @@ -34,7 +34,7 @@ ${CLEANUP_DRYRUN} && warning 'dry run mode is active' for repo in ${PKGREPOS[@]}; do for arch in ${ARCHES[@]}; do - if [ ! -f "${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT}" ]; then + if [[ ! -f ${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT} ]]; then continue fi # get a list of actual available package files @@ -43,7 +43,7 @@ for repo in ${PKGREPOS[@]}; do bsdtar -xOf "${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT}" | awk '/^%FILENAME%/{getline;print}' | sort > "${WORKDIR}/db-${repo}-${arch}" missing_pkgs=($(comm -13 "${WORKDIR}/repo-${repo}-${arch}" "${WORKDIR}/db-${repo}-${arch}")) - if [ ${#missing_pkgs[@]} -ge 1 ]; then + if (( ${#missing_pkgs[@]} >= 1 )); then error "Missing packages in [%s] (%s)..." "$repo" "$arch" for missing_pkg in ${missing_pkgs[@]}; do msg2 "${missing_pkg}" @@ -51,7 +51,7 @@ for repo in ${PKGREPOS[@]}; do fi old_pkgs=($(comm -23 "${WORKDIR}/repo-${repo}-${arch}" "${WORKDIR}/db-${repo}-${arch}")) - if [ ${#old_pkgs[@]} -ge 1 ]; then + if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old packages from [%s] (%s)..." "$repo" "$arch" for old_pkg in ${old_pkgs[@]}; do msg2 "${old_pkg}" @@ -67,7 +67,7 @@ find "$FTP_BASE/${PKGPOOL}" -name "*${PKGEXT}" -printf '%f\n' | sort > "${WORKDI find "${WORKDIR}" -maxdepth 1 -type f -name 'db-*' -exec cat {} \; | sort -u > "${WORKDIR}/db" old_pkgs=($(comm -23 "${WORKDIR}/pool" "${WORKDIR}/db")) -if [ ${#old_pkgs[@]} -ge 1 ]; then +if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old packages from package pool..." for old_pkg in ${old_pkgs[@]}; do msg2 "${old_pkg}" @@ -76,7 +76,7 @@ if [ ${#old_pkgs[@]} -ge 1 ]; then fi old_pkgs=($(find ${CLEANUP_DESTDIR} -type f -name "*${PKGEXT}" -mtime +${CLEANUP_KEEP} -printf '%f\n')) -if [ ${#old_pkgs[@]} -ge 1 ]; then +if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do msg2 "${old_pkg}" diff --git a/cron-jobs/integrity-check b/cron-jobs/integrity-check index 211a24b..f1e75ab 100755 --- a/cron-jobs/integrity-check +++ b/cron-jobs/integrity-check @@ -7,7 +7,7 @@ dirname="$(dirname $0)" script_lock -if [ $# -ne 1 ]; then +if (( $# != 1 )); then die "usage: ${0##*/} <mailto>" fi mailto=$1 diff --git a/cron-jobs/sourceballs b/cron-jobs/sourceballs index 9ab4e98..25a8abb 100755 --- a/cron-jobs/sourceballs +++ b/cron-jobs/sourceballs @@ -21,7 +21,7 @@ renice +10 -p $$ > /dev/null for repo in ${PKGREPOS[@]}; do for arch in ${ARCHES[@]}; do # Repo does not exist; skip it - if [ ! -f "${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT}" ]; then + if [[ ! -f ${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT} ]]; then continue fi bsdtar -xOf "${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT}" \ @@ -81,7 +81,7 @@ for repo in ${PKGREPOS[@]}; do mkdir -p -m0770 "${WORKDIR}/pkgbuilds/${repo}-${pkgarch}" arch_svn export -q "${SVNREPO}/${pkgbase}/repos/${repo}-${pkgarch}" \ "${WORKDIR}/pkgbuilds/${repo}-${pkgarch}/${pkgbase}" >/dev/null 2>&1 - if [ $? -ge 1 ]; then + if (( $? >= 1 )); then failedpkgs+=("${pkgbase}-${pkgver}${SRCEXT}") continue fi @@ -89,7 +89,7 @@ for repo in ${PKGREPOS[@]}; do # Build the actual source package pushd "${WORKDIR}/pkgbuilds/${repo}-${pkgarch}/${pkgbase}" >/dev/null makepkg --nocolor --allsource --ignorearch --skippgpcheck --config "${dirname}/makepkg.conf" >"${WORKDIR}/${pkgbase}.log" 2>&1 - if [ $? -eq 0 ] && [ -f "${pkgbase}-${pkgver}${SRCEXT}" ]; then + if (( $? == 0 )) && [[ -f ${pkgbase}-${pkgver}${SRCEXT} ]]; then mv_acl "${pkgbase}-${pkgver}${SRCEXT}" "${FTP_BASE}/${SRCPOOL}/${pkgbase}-${pkgver}${SRCEXT}" # Avoid creating the same source package for every arch echo "${pkgbase}-${pkgver}${SRCEXT}" >> "${WORKDIR}/available-src-pkgs" @@ -121,7 +121,7 @@ find "${WORKDIR}" -maxdepth 1 -type f -name 'expected-src-pkgs' -exec cat {} \; find "${WORKDIR}" -maxdepth 1 -type f -name 'available-src-pkgs' -exec cat {} \; | sort -u > "${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 +if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old source packages..." ${SOURCE_CLEANUP_DRYRUN} && warning 'dry run mode is active' for old_pkg in ${old_pkgs[@]}; do @@ -134,7 +134,7 @@ if [ ${#old_pkgs[@]} -ge 1 ]; then fi old_pkgs=($(find ${SOURCE_CLEANUP_DESTDIR} -type f -name "*${SRCEXT}" -mtime +${SOURCE_CLEANUP_KEEP} -printf '%f\n')) -if [ ${#old_pkgs[@]} -ge 1 ]; then +if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old source packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do msg2 "${old_pkg}" @@ -142,7 +142,7 @@ if [ ${#old_pkgs[@]} -ge 1 ]; then done fi -if [ -f "${WORKDIR}/makepkg-fail.log" ]; then +if [[ -f ${WORKDIR}/makepkg-fail.log ]]; then msg "Log of failed packages" cat "${WORKDIR}/makepkg-fail.log" fi diff --git a/cron-jobs/update-web-db b/cron-jobs/update-web-db index 23af067..c859ce0 100755 --- a/cron-jobs/update-web-db +++ b/cron-jobs/update-web-db @@ -27,7 +27,7 @@ renice +5 -p $$ > /dev/null echo "%s: Updating DB at %s" "$cmd" "$(date)" >> "${LOGOUT}" # source our virtualenv if it exists -if [ -f "$ENVPATH" ]; then +if [[ -f "$ENVPATH" ]]; then . "$ENVPATH" fi @@ -47,7 +47,7 @@ for repo in ${REPOS[@]}; do for arch in ${ARCHES[@]}; do repo_lock ${repo} ${arch} || exit 1 dbfile="/srv/ftp/${repo}/os/${arch}/${repo}${dbfileext}" - if [ -f "${dbfile}" ]; then + if [[ -f ${dbfile} ]]; then mkdir -p "${WORKDIR}/${repo}/${arch}" cp "${dbfile}" "${WORKDIR}/${repo}/${arch}/${repo}${dbfileext}" fi @@ -60,7 +60,7 @@ pushd $SPATH >/dev/null for repo in ${REPOS[@]}; do for arch in ${ARCHES[@]}; do dbcopy="${WORKDIR}/${repo}/${arch}/${repo}${dbfileext}" - if [ -f "${dbcopy}" ]; then + if [[ -f ${dbcopy} ]]; then echo "Updating ${repo}-${arch}" >> "${LOGOUT}" ./manage.py reporead ${flags} ${arch} "${dbcopy}" >> "${LOGOUT}" 2>&1 echo "" >> "${LOGOUT}"
Catch some cases that were missed in the previous run. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- This patch is new + refactor some changes from: ftpdir-cleanup,sourceballs: replace external find command with bash globbing cron-jobs/devlist-mailer | 6 +++--- cron-jobs/ftpdir-cleanup | 14 +++++++------- cron-jobs/integrity-check | 2 +- cron-jobs/sourceballs | 12 ++++++------ cron-jobs/update-web-db | 6 +++--- 5 files changed, 20 insertions(+), 20 deletions(-)