[pacman-dev] [PATCH 1/2] Refactor check_sanity, to give it some sanity of its own
Dave Reisner
d at falconindy.com
Sat Aug 2 20:25:03 EDT 2014
On Sat, Aug 02, 2014 at 11:54:30AM -0400, Dave Reisner wrote:
> Break apart each of the blocks into their own separate functions. And,
> instead of the hand crafted eval statements, reuse the logic from
> pkgbuild-introspection[0] to abstract away the complexities of parsing
> bash.
>
> This commit fixes at least 3 bugs in check_sanity, and one other:
>
> 1) The wrong variable is shown for the error which would be thrown
> when, e.g. pkgname=('foopkg' 'bar at pkg')
> 2) The "arch" variable is not sanity checked when the PKGBUILD has
> package_$pkgname() instead of package().
> 3) Avoid linting pkgrel/epoch in package functions (because we don't
> actually support these overrides).
Looking at get_full_version, I guess I have this wrong. I was thrown off
by the last line that makepkg outputs, "Finished making: ...", where it
reports the global pkgver, which might be irrelevant to the package with
the package override.
> 4) The "arch" variable is leaked when processing arch overrides in
> packages. For example:
>
> pkgname=(foo libfoo)
> arch=('i686' 'x86_64')
> ....
>
> package_foo() {
> arch=('any')
> :
> }
>
> package_libfoo() {
> :
> }
>
> This leaks arch=('any') into package_libfoo. Reversing the order of
> pkgname will mask this bug.
>
> Lastly, there's some string changes here which should help to clarify
> a few errors emitted in the linting process.
>
> [0] https://github.com/falconindy/pkgbuild-introspection
> ---
> scripts/makepkg.sh.in | 391 ++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 284 insertions(+), 107 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index ee70e43..baa3720 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -2202,18 +2202,89 @@ have_function() {
> declare -f "$1" >/dev/null
> }
>
> -check_sanity() {
> - # check for no-no's in the build script
> - local i
> - local ret=0
> - for i in 'pkgname' 'pkgrel'; do
> - if [[ -z ${!i} ]]; then
> - error "$(gettext "%s is not allowed to be empty.")" "$i"
> - ret=1
> - fi
> - done
> +array_build() {
> + eval "$1=(\"\${$2[@]}\")"
> +}
> +
> +funcgrep() {
> + { declare -f "$1" || declare -f package; } 2>/dev/null | grep -E "$2"
> +}
> +
> +extract_global_var() {
> + # $1: variable name
> + # $2: multivalued
> + # $3: name of output var
> +
> + local attr=$1 isarray=$2 outputvar=$3
> +
> + if (( isarray )); then
> + array_build ref "$attr"
> + [[ ${ref[@]} ]] && array_build "$outputvar" "$attr"
> + else
> + [[ -v $attr ]] && printf -v "$outputvar" %s "${!attr}"
> + fi
> +}
> +
> +extract_function_var() {
> + # $1: function name
> + # $2: variable name
> + # $3: multivalued
> + # $4: name of output var
> +
> + local funcname=$1 attr=$2 isarray=$3 outputvar=$4 attr_regex= decl= r=1
> +
> + if (( isarray )); then
> + printf -v attr_regex '^[[:space:]]*(declare( -[[:alpha:]])*)? %q\+?=\(' "$2"
> + else
> + printf -v attr_regex '^[[:space:]]*(declare( -[[:alpha:]])*)? %q\+?=[^(]' "$2"
> + fi
> +
> + while read -r; do
> + # strip leading whitespace and any usage of declare
> + decl=${REPLY##*([[:space:]])?(declare +(-+([[:alpha:]]) ))}
> + eval "${decl/#$attr/$outputvar}"
> +
> + # entering this loop at all means we found a match, so notify the caller.
> + r=0
> + done < <(funcgrep "$funcname" "$attr_regex")
> +
> + return $r
> +}
> +
> +pkgbuild_get_attribute() {
> + # $1: package name
> + # $2: attribute name
> + # $3: name of output var
> + # $4: multivalued
> +
> + local pkgname=$1 attrname=$2 outputvar=$3 isarray=$4
> +
> + printf -v "$outputvar" %s ''
> +
> + if [[ $pkgname ]]; then
> + extract_global_var "$attrname" "$isarray" "$outputvar"
> + extract_function_var "package_$pkgname" "$attrname" "$isarray" "$outputvar"
> + else
> + extract_global_var "$attrname" "$isarray" "$outputvar"
> + fi
> +}
> +
> +lint_pkgbase() {
> + if [[ ${pkgbase:0:1} = "-" ]]; then
> + error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgname"
> + return 1
> + fi
> +}
> +
> +lint_pkgname() {
> + local ret=0 i
>
> for i in "${pkgname[@]}"; do
> + if [[ -z $i ]]; then
> + error "$(gettext "%s is not allowed to be empty.")" "pkgname"
> + ret=1
> + continue
> + fi
> if [[ ${i:0:1} = "-" ]]; then
> error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgname"
> ret=1
> @@ -2224,155 +2295,261 @@ check_sanity() {
> fi
> if [[ $i = *[^[:alnum:]+_. at -]* ]]; then
> error "$(gettext "%s contains invalid characters: '%s'")" \
> - 'pkgname' "${pkgname//[[:alnum:]+_. at -]}"
> + 'pkgname' "${i//[[:alnum:]+_. at -]}"
> ret=1
> fi
> done
>
> - if [[ ${pkgbase:0:1} = "-" ]]; then
> - error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgbase"
> + return $ret
> +}
> +
> +lint_pkgrel() {
> + local ret=0
> +
> + if [[ -z $pkgrel ]]; then
> + error "$(gettext "%s is not allowed to be empty.")" "pkgrel"
> ret=1
> fi
>
> - if (( ! PKGVERFUNC )) ; then
> - check_pkgver || ret=1
> + if [[ $pkgrel != +([0-9])?(.+([0-9])) ]]; then
> + error "$(gettext "%s must be a decimal, not %s.")" "pkgrel" "$pkgrel"
> + ret=1
> fi
>
> - awk -F'=' '$1 ~ /^[[:space:]]*pkgrel$/' "$BUILDFILE" | sed "s/[[:space:]]*#.*//" |
> - while IFS='=' read -r _ i; do
> - eval i=\"$(sed 's/^\(['\''"]\)\(.*\)\1$/\2/' <<< "${i%%+([[:space:]])}")\"
> - if [[ $i != +([0-9])?(.+([0-9])) ]]; then
> - error "$(gettext "%s must be a decimal.")" "pkgrel"
> - return 1
> - fi
> - done || ret=1
> + return $ret
> +}
>
> - awk -F'=' '$1 ~ /^[[:space:]]*epoch$/' "$BUILDFILE" |
> - while IFS='=' read -r _ i; do
> - eval i=\"$(sed 's/^\(['\''"]\)\(.*\)\1$/\2/' <<< "${i%%+([[:space:]])}")\"
> - if [[ $i != *([[:digit:]]) ]]; then
> - error "$(gettext "%s must be an integer.")" "epoch"
> - return 1
> - fi
> - done || ret=1
> +lint_pkgver() {
> + if (( PKGVERFUNC )); then
> + # defer check to after getting version from pkgver function
> + return 0
> + fi
> +
> + check_pkgver
> +}
> +
> +lint_epoch() {
> + if [[ $epoch != *([[:digit:]]) ]]; then
> + error "$(gettext "%s must be an integer, not %s.")" "epoch" "$e"
> + return 1
> + fi
> +}
> +
> +lint_arch() {
> + local name list=("${@:-"${arch[@]}"}")
>
> - if [[ $arch != 'any' ]]; then
> - if ! in_array $CARCH "${arch[@]}"; then
> + if [[ $list == 'any' ]]; then
> + return 0
> + fi
> +
> + if ! in_array "$CARCH" "${list[@]}" && (( ! IGNOREARCH )); then
> + error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgbase" "$CARCH"
> + plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT"
> + plain "$(gettext "such as %s.")" "arch=('$CARCH')"
> + return 1
> + fi
> +
> + for name in "${pkgname[@]}"; do
> + pkgbuild_get_attribute "$name" 'arch' list 1
> + if [[ $list && $list != 'any' ]] && ! in_array $CARCH "${list[@]}"; then
> if (( ! IGNOREARCH )); then
> - error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgbase" "$CARCH"
> - plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT"
> - plain "$(gettext "such as %s.")" "arch=('$CARCH')"
> + error "$(gettext "%s is not available for the '%s' architecture.")" "$name" "$CARCH"
> ret=1
> fi
> fi
> - fi
> + done
> +}
>
> - if (( ${#pkgname[@]} > 1 )); then
> - for i in ${pkgname[@]}; do
> - local arch_list=""
> - eval $(declare -f package_${i} | sed -n 's/\(^[[:space:]]*arch=\)/arch_list=/p')
> - if [[ ${arch_list[@]} && ${arch_list} != 'any' ]]; then
> - if ! in_array $CARCH "${arch_list[@]}"; then
> - if (( ! IGNOREARCH )); then
> - error "$(gettext "%s is not available for the '%s' architecture.")" "$i" "$CARCH"
> - ret=1
> - fi
> - fi
> - fi
> - done
> - fi
> +lint_provides() {
> + local list name provides_list ret=0
>
> - local provides_list=()
> - eval $(awk '/^[[:space:]]*provides=/,/\)/' "$BUILDFILE" | \
> - sed -e "s/provides=/provides_list+=/" -e "s/#.*//" -e 's/\\$//')
> - for i in ${provides_list[@]}; do
> - if [[ $i == *['<>']* ]]; then
> + provides_list=("${provides[@]}")
> + for name in "${pkgname[@]}"; do
> + extract_function_var "package_$name" provides 1 list
> + provides_list+=("${list[@]}")
> + done
> +
> + for provide in "${provides_list[@]}"; do
> + if [[ $provide == *['<>']* ]]; then
> error "$(gettext "%s array cannot contain comparison (< or >) operators.")" "provides"
> ret=1
> fi
> done
>
> - local backup_list=()
> - eval $(awk '/^[[:space:]]*backup=/,/\)/' "$BUILDFILE" | \
> - sed -e "s/backup=/backup_list+=/" -e "s/#.*//" -e 's/\\$//')
> - for i in "${backup_list[@]}"; do
> - if [[ ${i:0:1} = "/" ]]; then
> - error "$(gettext "%s entry should not contain leading slash : %s")" "backup" "$i"
> + return $ret
> +}
> +
> +lint_backup() {
> + local list name backup_list ret=0
> +
> + backup_list=("${backup[@]}")
> + for name in "${pkgname[@]}"; do
> + extract_function_var "package_$name" backup 1 list
> + backup_list+=("${list[@]}")
> + done
> +
> + for name in "${backup_list[@]}"; do
> + if [[ ${name:0:1} = "/" ]]; then
> + error "$(gettext "%s entry should not contain leading slash : %s")" "backup" "$name"
> ret=1
> fi
> done
>
> - local optdepends_list=()
> - eval $(awk '/^[[:space:]]*optdepends=\(/,/\)[[:space:]]*(#.*)?$/' "$BUILDFILE" | \
> - sed -e "s/optdepends=/optdepends_list+=/" -e "s/#.*//" -e 's/\\$//')
> - for i in "${optdepends_list[@]}"; do
> - local pkg=${i%%:[[:space:]]*}
> + return $ret
> +}
> +
> +lint_optdepends() {
> + local list name optdepends_list ret=0
> +
> + optdepends_list=("${optdepends[@]}")
> + for name in "${pkgname[@]}"; do
> + extract_function_var "package_$name" optdepends 1 list
> + optdepends_list+=("${list[@]}")
> + done
> +
> + for name in "${optdepends_list[@]}"; do
> + local pkg=${name%%:[[:space:]]*}
> # the '-' character _must_ be first or last in the character range
> if [[ $pkg != +([-[:alnum:]><=.+_:]) ]]; then
> - error "$(gettext "Invalid syntax for %s : '%s'")" "optdepend" "$i"
> + error "$(gettext "Invalid syntax for %s: '%s'")" "optdepend" "$name"
> ret=1
> fi
> done
>
> - for i in 'changelog' 'install'; do
> - local file
> - while read -r file; do
> - # evaluate any bash variables used
> - eval file=\"$(sed 's/^\(['\''"]\)\(.*\)\1$/\2/' <<< "$file")\"
> - if [[ $file && ! -f $file ]]; then
> - error "$(gettext "%s file (%s) does not exist.")" "$i" "$file"
> - ret=1
> - fi
> - done < <(sed -n "s/^[[:space:]]*$i=//p" "$BUILDFILE")
> + return $ret
> +}
> +
> +check_files_exist() {
> + local kind=$1 files=("${@:2}") file ret
> +
> + for file in "${files[@]}"; do
> + if [[ $file && ! -f $file ]]; then
> + error "$(gettext "%s file (%s) does not exist or is not a regular file.")" \
> + "$kind" "$file"
> + ret=1
> + fi
> + done
> +
> + return $ret
> +}
> +
> +lint_install() {
> + local list file name install_list ret=0
> +
> + install_list=("${install[@]}")
> + for name in "${pkgname[@]}"; do
> + extract_function_var "package_$name" install 0 file
> + install_list+=("$file")
> + done
> +
> + check_files_exist 'install' "${install_list[@]}"
> +}
> +
> +lint_changelog() {
> + local list name file changelog_list ret=0
> +
> + changelog_list=("${changelog[@]}")
> + for name in "${pkgname[@]}"; do
> + extract_function_var "package_$name" changelog 0 file
> + changelog_list+=("$file")
> + done
> +
> + check_files_exist 'changelog' "${changelog_list[@]}"
> +}
> +
> +lint_options() {
> + local ret=0 list name kopt options_list
> +
> + options_list=("${options[@]}")
> + for name in "${pkgname[@]}"; do
> + extract_function_var "package_$name" options 1 list
> + options_list+=("${list[@]}")
> done
>
> - local valid_options=1
> - local known kopt options_list
> - eval $(awk '/^[[:space:]]*options=/,/\)/' "$BUILDFILE" | \
> - sed -e "s/options=/options_list+=/" -e "s/#.*//" -e 's/\\$//')
> - for i in ${options_list[@]}; do
> - known=0
> + for i in "${options_list[@]}"; do
> # check if option matches a known option or its inverse
> - for kopt in ${packaging_options[@]} ${other_options[@]}; do
> - if [[ ${i} = "${kopt}" || ${i} = "!${kopt}" ]]; then
> - known=1
> + for kopt in "${packaging_options[@]}" "${other_options[@]}"; do
> + if [[ $i = "$kopt" || $i = "!$kopt" ]]; then
> + # continue to the next $i
> + continue 2
> fi
> done
> - if (( ! known )); then
> - error "$(gettext "%s array contains unknown option '%s'")" "options" "$i"
> - valid_options=0
> - fi
> - done
> - if (( ! valid_options )); then
> +
> + error "$(gettext "%s array contains unknown option '%s'")" "options" "$i"
> ret=1
> + done
> +
> + return $ret
> +}
> +
> +lint_source() {
> + local idx=("${!source[@]}")
> +
> + if (( ${#source[*]} > 0 && (idx[-1] + 1) != ${#source[*]} )); then
> + error "$(gettext "Sparse arrays are not allowed for source")"
> + return 1
> fi
> +}
> +
> +lint_pkglist() {
> + local i ret=0
> +
> + for i in "${PKGLIST[@]}"; do
> + if ! in_array "$i" "${pkgname[@]}"; then
> + error "$(gettext "Requested package %s is not provided in %s")" "$i" "$BUILDFILE"
> + ret=1
> + fi
> + done
> +
> + return $ret
> +}
> +
> +lint_packagefn() {
> + local i ret=0
>
> if (( ${#pkgname[@]} == 1 )); then
> - if have_function build && ! ( have_function package || have_function package_${pkgname}); then
> + if have_function 'build' && ! { have_function 'package' || have_function "package_$pkgname"; }; then
> error "$(gettext "Missing %s function in %s")" "package()" "$BUILDFILE"
> ret=1
> fi
> else
> - for i in ${pkgname[@]}; do
> - if ! have_function package_${i}; then
> + for i in "${pkgname[@]}"; do
> + if ! have_function "package_$i"; then
> error "$(gettext "Missing %s function for split package '%s'")" "package_$i()" "$i"
> ret=1
> fi
> done
> fi
>
> - for i in ${PKGLIST[@]}; do
> - if ! in_array $i ${pkgname[@]}; then
> - error "$(gettext "Requested package %s is not provided in %s")" "$i" "$BUILDFILE"
> - ret=1
> - fi
> - done
> + return $ret
> +}
>
> - local idx=("${!source[@]}")
> - if (( ${#source[*]} > 0 && (idx[-1] + 1) != ${#source[*]} )); then
> - error "$(gettext "Sparse arrays are not allowed for source")"
> - ret=1
> - fi
> +check_sanity() {
> + # check for no-no's in the build script
> + local ret=0
> + local lintfn lint_functions
> +
> + lint_functions=(
> + lint_pkgbase
> + lint_pkgname
> + lint_pkgver
> + lint_pkgrel
> + lint_epoch
> + lint_arch
> + lint_provides
> + lint_backup
> + lint_optdepends
> + lint_changelog
> + lint_install
> + lint_options
> + lint_packagefn
> + lint_pkglist
> + lint_source
> + )
> +
> + for lintfn in "${lint_functions[@]}"; do
> + "$lintfn" || ret=1
> + done
>
> return $ret
> }
> --
> 2.0.3
More information about the pacman-dev
mailing list