[pacman-dev] [PATCH 1/2] Refactor check_sanity, to give it some sanity of its own
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@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). 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:]+_.@-]* ]]; then error "$(gettext "%s contains invalid characters: '%s'")" \ - 'pkgname' "${pkgname//[[:alnum:]+_.@-]}" + 'pkgname' "${i//[[:alnum:]+_.@-]}" 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
This introduces support for architecutre-specific conflicts, depends, optdepends, makedepends, replaces, and conflicts by appending "_$CARCH" to the array name. For example, in the global section: arch=('i686' 'x86_64') depends=('foo') depends_x86_64=('bar') This will generate depends of 'foo' and 'bar' on x86_64, but only 'foo' on i686. Moreover, this is supported in the package functions with the same heuristics as the generic names, e.g. ... arch=('i686' 'x86_64') depends=('foo') ... package_somepkg() { depends_x86_64=('bar') ... } Again, will cause x86_64 to have depends of 'foo' and 'bar', but only 'foo' for i686. --- v2: 1) extended this to a bunch more attributes -- some of them I can't necessarily reason an immediate use case for, but conceptually could have their uses as arch-specific attributes. 2) Added documentation 3) Added check_sanity support post mega-refactoring. doc/PKGBUILD.5.txt | 21 +++++++++++++++++++++ scripts/makepkg.sh.in | 46 +++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/doc/PKGBUILD.5.txt b/doc/PKGBUILD.5.txt index 17e8af2..5cebbbd 100644 --- a/doc/PKGBUILD.5.txt +++ b/doc/PKGBUILD.5.txt @@ -183,17 +183,26 @@ If the dependency name appears to be a library (ends with .so), makepkg will try to find a binary that depends on the library in the built package and append the version needed by the binary. Appending the version yourself disables automatic detection. ++ +Architecture-specific depends can be added by appending an underscore and the +architecture name e.g., 'depends_x86_64=()'. *makedepends (array)*:: An array of packages this package depends on to build but are not needed at runtime. Packages in this list follow the same format as depends. ++ +Architecture-specific makedepends can be added by appending an underscore and +the architecture name e.g., 'makedepends_x86_64=()'. *checkdepends (array)*:: An array of packages this package depends on to run its test suite but are not needed at runtime. Packages in this list follow the same format as depends. These dependencies are only considered when the check() function is present and is to be run by makepkg. ++ +Architecture-specific checkdepends can be added by appending an underscore and +the architecture name e.g., 'checkdepends_x86_64=()'. *optdepends (array)*:: An array of packages (and accompanying reasons) that are not essential for @@ -203,12 +212,18 @@ disables automatic detection. for specifying optdepends is: optdepends=('fakeroot: for makepkg usage as normal user') ++ +Architecture-specific optdepends can be added by appending an underscore and +the architecture name e.g., 'optdepends_x86_64=()'. *conflicts (array)*:: An array of packages that will conflict with this package (i.e. they cannot both be installed at the same time). This directive follows the same format as depends. Versioned conflicts are supported using the operators as described in `depends`. ++ +Architecture-specific conflicts can be added by appending an underscore and +the architecture name e.g., 'conflicts_x86_64=()'. *provides (array)*:: An array of ``virtual provisions'' this package provides. This allows @@ -224,6 +239,9 @@ only specific versions of a package may be provided. If the provision name appears to be a library (ends with .so), makepkg will try to find the library in the built package and append the correct version. Appending the version yourself disables automatic detection. ++ +Architecture-specific provides can be added by appending an underscore and +the architecture name e.g., 'provides_x86_64=()'. *replaces (array)*:: An array of packages this package should replace. This can be used @@ -234,6 +252,9 @@ version. Appending the version yourself disables automatic detection. + Sysupgrade is currently the only pacman operation that utilizes this field. A normal sync or upgrade will not use its value. ++ +Architecture-specific replaces can be added by appending an underscore and +the architecture name e.g., 'replaces_x86_64=()'. *options (array)*:: This array allows you to override some of makepkg's default behavior diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index baa3720..564ae1f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1487,6 +1487,24 @@ source_safe() { shopt -s extglob } +merge_arch_attrs() { + local attr supported_attrs=( + provides conflicts depends replaces optdepends + makedepends checkdepends) + + for attr in "${supported_attrs[@]}"; do + eval "$attr+=(\"\${${attr}_$CARCH[@]}\")" + done + + # ensure that calling this function is idempotent. + unset -v "${supported_attrs[@]/%/_$CARCH}" +} + +source_buildfile() { + source_safe "$@" + merge_arch_attrs +} + run_function_safe() { local restoretrap @@ -1905,6 +1923,8 @@ write_pkginfo() { local size="$(@DUPATH@ @DUFLAGS@)" size="$(( ${size%%[^0-9]*} * 1024 ))" + merge_arch_attrs + msg2 "$(gettext "Generating %s file...")" ".PKGINFO" printf "# Generated by makepkg %s\n" "$makepkg_version" printf "# using %s\n" "$(fakeroot -v)" @@ -2361,12 +2381,22 @@ lint_arch() { } lint_provides() { - local list name provides_list ret=0 + local a list name provides_list ret=0 provides_list=("${provides[@]}") + for a in "${arch[@]}"; do + array_build list "provides_$a" + provides_list+=("${list[@]}") + done + for name in "${pkgname[@]}"; do extract_function_var "package_$name" provides 1 list provides_list+=("${list[@]}") + + for a in "${arch[@]}"; do + extract_function_var "package_$name" "provides_$a" 1 list + provides_list+=("${list[@]}") + done done for provide in "${provides_list[@]}"; do @@ -2399,12 +2429,22 @@ lint_backup() { } lint_optdepends() { - local list name optdepends_list ret=0 + local a list name optdepends_list ret=0 optdepends_list=("${optdepends[@]}") + for a in "${arch[@]}"; do + array_build list "optdepends_$a" + optdepends_list+=("${list[@]}") + done + for name in "${pkgname[@]}"; do extract_function_var "package_$name" optdepends 1 list optdepends_list+=("${list[@]}") + + for a in "${arch[@]}"; do + extract_function_var "package_$name" "optdepends_$a" 1 list + optdepends_list+=("${list[@]}") + done done for name in "${optdepends_list[@]}"; do @@ -3091,7 +3131,7 @@ else if [[ ${BUILDFILE:0:1} != "/" ]]; then BUILDFILE="$startdir/$BUILDFILE" fi - source_safe "$BUILDFILE" + source_buildfile "$BUILDFILE" fi # set defaults if they weren't specified in buildfile -- 2.0.3
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@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:]+_.@-]* ]]; then error "$(gettext "%s contains invalid characters: '%s'")" \ - 'pkgname' "${pkgname//[[:alnum:]+_.@-]}" + 'pkgname' "${i//[[:alnum:]+_.@-]}" 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
On 03/08/14 10:25, Dave Reisner wrote:
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@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.
Currently we can override pkgver, pkgrel and epoch. This was on the bases of a single piece of software which was really two pieces of software bundled in the same tarball with different versions. I am think we could just remove it and simplify a lot of things... Opinions? Allan
On 03/08/14 01:54, 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:
Can you clarify these bugs for me?
1) The wrong variable is shown for the error which would be thrown when, e.g. pkgname=('foopkg' 'bar@pkg')
What error? This pkgname actually works here...
2) The "arch" variable is not sanity checked when the PKGBUILD has package_$pkgname() instead of package().
Do you mean with an override in package_$pkgname()?
3) Avoid linting pkgrel/epoch in package functions (because we don't actually support these overrides).
As said in my previous mail, I vote to get rid of overriding pkg{ver,rel} and epoch.
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.
Huh? This works for me: foo-1-1-any.pkg.tar.xz libfoo-1-1-x86_64.pkg.tar.xz Allan
On 03/08/14 01:54, Dave Reisner wrote:
This introduces support for architecutre-specific conflicts, depends, optdepends, makedepends, replaces, and conflicts by appending "_$CARCH" to the array name. For example, in the global section:
arch=('i686' 'x86_64') depends=('foo') depends_x86_64=('bar')
This will generate depends of 'foo' and 'bar' on x86_64, but only 'foo' on i686. Moreover, this is supported in the package functions with the same heuristics as the generic names, e.g.
... arch=('i686' 'x86_64') depends=('foo') ...
package_somepkg() { depends_x86_64=('bar')
... }
Again, will cause x86_64 to have depends of 'foo' and 'bar', but only 'foo' for i686. --- v2: 1) extended this to a bunch more attributes -- some of them I can't necessarily reason an immediate use case for, but conceptually could have their uses as arch-specific attributes. 2) Added documentation 3) Added check_sanity support post mega-refactoring.
doc/PKGBUILD.5.txt | 21 +++++++++++++++++++++ scripts/makepkg.sh.in | 46 +++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/doc/PKGBUILD.5.txt b/doc/PKGBUILD.5.txt index 17e8af2..5cebbbd 100644 --- a/doc/PKGBUILD.5.txt +++ b/doc/PKGBUILD.5.txt @@ -183,17 +183,26 @@ If the dependency name appears to be a library (ends with .so), makepkg will try to find a binary that depends on the library in the built package and append the version needed by the binary. Appending the version yourself disables automatic detection. ++ +Architecture-specific depends can be added by appending an underscore and the +architecture name e.g., 'depends_x86_64=()'.
Do we want to repeat this sentence many times or add a single section detailing architecture specific additions. Also, despite "added" there, I do not find it clear that these are overrides. Perhaps "Additional architecture-specific..." would help. If this was in its own section, it could be expanded for clarity.
On Sun, Aug 03, 2014 at 06:39:54PM +1000, Allan McRae wrote:
On 03/08/14 01:54, 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:
Can you clarify these bugs for me?
1) The wrong variable is shown for the error which would be thrown when, e.g. pkgname=('foopkg' 'bar@pkg')
What error? This pkgname actually works here...
Sorry, I found this error my static analysis, and then randomly chose a name which turns out to be valid (I recall we made an exception for @). If you have something like pkgname=('foopkg' 'bar^pkg') you'll get the error: ==> ERROR: pkgname contains invalid characters: '' Whereas, if you use pkgname=('bar^pkg' 'foopkg'), or apply this patch, you'll see: ==> ERROR: pkgname contains invalid characters: '^'
2) The "arch" variable is not sanity checked when the PKGBUILD has package_$pkgname() instead of package().
Do you mean with an override in package_$pkgname()?
Actually, it seems to be the case that arch is never sanity checked at all for a singular package -- has nothing to do with how the package function is named. e.g.: pkgname=foo arch=('i686' 'x86_64') ... package_foo() { # or name this package() arch=('invalid') ... } This should explode, but doesn't, and happily makes a package for x86_64 (since my host is x86_64). After patch: ==> ERROR: foo is not available for the 'x86_64' architecture.
3) Avoid linting pkgrel/epoch in package functions (because we don't actually support these overrides).
As said in my previous mail, I vote to get rid of overriding pkg{ver,rel} and epoch.
I'm fine with this. It doesn't make a whole lot of sense to me, and no packages in the Arch repos currently override pkgver/pkgrel/epoch in package functions.
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.
Huh? This works for me:
foo-1-1-any.pkg.tar.xz libfoo-1-1-x86_64.pkg.tar.xz
I swear I saw this, but I can't replicate the problem anymore. The backup/restore variable logic seems relatively sane, too.
On Sun, Aug 03, 2014 at 06:50:19PM +1000, Allan McRae wrote:
On 03/08/14 01:54, Dave Reisner wrote:
This introduces support for architecutre-specific conflicts, depends, optdepends, makedepends, replaces, and conflicts by appending "_$CARCH" to the array name. For example, in the global section:
arch=('i686' 'x86_64') depends=('foo') depends_x86_64=('bar')
This will generate depends of 'foo' and 'bar' on x86_64, but only 'foo' on i686. Moreover, this is supported in the package functions with the same heuristics as the generic names, e.g.
... arch=('i686' 'x86_64') depends=('foo') ...
package_somepkg() { depends_x86_64=('bar')
... }
Again, will cause x86_64 to have depends of 'foo' and 'bar', but only 'foo' for i686. --- v2: 1) extended this to a bunch more attributes -- some of them I can't necessarily reason an immediate use case for, but conceptually could have their uses as arch-specific attributes. 2) Added documentation 3) Added check_sanity support post mega-refactoring.
doc/PKGBUILD.5.txt | 21 +++++++++++++++++++++ scripts/makepkg.sh.in | 46 +++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/doc/PKGBUILD.5.txt b/doc/PKGBUILD.5.txt index 17e8af2..5cebbbd 100644 --- a/doc/PKGBUILD.5.txt +++ b/doc/PKGBUILD.5.txt @@ -183,17 +183,26 @@ If the dependency name appears to be a library (ends with .so), makepkg will try to find a binary that depends on the library in the built package and append the version needed by the binary. Appending the version yourself disables automatic detection. ++ +Architecture-specific depends can be added by appending an underscore and the +architecture name e.g., 'depends_x86_64=()'.
Do we want to repeat this sentence many times or add a single section detailing architecture specific additions.
I think it's more clear to document it directly alongside the relevant variables instead of breaking out a new section.
Also, despite "added" there, I do not find it clear that these are overrides. Perhaps "Additional architecture-specific..." would help. If this was in its own section, it could be expanded for clarity.
It's not clear that they're overrides because they *aren't* overrides. If you specify: arch=('i686' 'x86_64') depends=('libfoo') depends_x86_64=('lib32-libbar') Then, you create an i686 package with depends of 'libfoo' only, and an x86_64 package with depends of 'libfoo' *and* 'lib32-libbar'.
On 03/08/14 22:57, Dave Reisner wrote:
On Sun, Aug 03, 2014 at 06:50:19PM +1000, Allan McRae wrote:
On 03/08/14 01:54, Dave Reisner wrote:
This introduces support for architecutre-specific conflicts, depends, optdepends, makedepends, replaces, and conflicts by appending "_$CARCH" to the array name. For example, in the global section:
arch=('i686' 'x86_64') depends=('foo') depends_x86_64=('bar')
This will generate depends of 'foo' and 'bar' on x86_64, but only 'foo' on i686. Moreover, this is supported in the package functions with the same heuristics as the generic names, e.g.
... arch=('i686' 'x86_64') depends=('foo') ...
package_somepkg() { depends_x86_64=('bar')
... }
Again, will cause x86_64 to have depends of 'foo' and 'bar', but only 'foo' for i686. --- v2: 1) extended this to a bunch more attributes -- some of them I can't necessarily reason an immediate use case for, but conceptually could have their uses as arch-specific attributes. 2) Added documentation 3) Added check_sanity support post mega-refactoring.
doc/PKGBUILD.5.txt | 21 +++++++++++++++++++++ scripts/makepkg.sh.in | 46 +++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/doc/PKGBUILD.5.txt b/doc/PKGBUILD.5.txt index 17e8af2..5cebbbd 100644 --- a/doc/PKGBUILD.5.txt +++ b/doc/PKGBUILD.5.txt @@ -183,17 +183,26 @@ If the dependency name appears to be a library (ends with .so), makepkg will try to find a binary that depends on the library in the built package and append the version needed by the binary. Appending the version yourself disables automatic detection. ++ +Architecture-specific depends can be added by appending an underscore and the +architecture name e.g., 'depends_x86_64=()'.
Do we want to repeat this sentence many times or add a single section detailing architecture specific additions.
I think it's more clear to document it directly alongside the relevant variables instead of breaking out a new section.
Also, despite "added" there, I do not find it clear that these are overrides. Perhaps "Additional architecture-specific..." would help. If this was in its own section, it could be expanded for clarity.
It's not clear that they're overrides because they *aren't* overrides.
Oops... I meant it is not clear that the aren't overrides. Hence my suggestion "Additional architec..". or maybe: Architecture-specific depends can be added to the specified dependencies by appending an ...
On 03/08/14 22:54, Dave Reisner wrote:
On Sun, Aug 03, 2014 at 06:39:54PM +1000, Allan McRae wrote:
On 03/08/14 01:54, 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:
Can you clarify these bugs for me?
1) The wrong variable is shown for the error which would be thrown when, e.g. pkgname=('foopkg' 'bar@pkg')
What error? This pkgname actually works here...
Sorry, I found this error my static analysis, and then randomly chose a name which turns out to be valid (I recall we made an exception for @). If you have something like pkgname=('foopkg' 'bar^pkg') you'll get the error:
==> ERROR: pkgname contains invalid characters: ''
Whereas, if you use pkgname=('bar^pkg' 'foopkg'), or apply this patch, you'll see:
==> ERROR: pkgname contains invalid characters: '^'
2) The "arch" variable is not sanity checked when the PKGBUILD has package_$pkgname() instead of package().
Do you mean with an override in package_$pkgname()?
Actually, it seems to be the case that arch is never sanity checked at all for a singular package -- has nothing to do with how the package function is named. e.g.:
pkgname=foo arch=('i686' 'x86_64') ...
package_foo() { # or name this package() arch=('invalid')
... }
This should explode, but doesn't, and happily makes a package for x86_64 (since my host is x86_64). After patch:
==> ERROR: foo is not available for the 'x86_64' architecture.
Ah... I see that check_sanity tested if the number of packages > 1 because at that stage we have not tested whether package_foo() or package() is used. And now also see we can override stuff in package(), so the #packages > 1 check was useless. I'll take a look at the patch itself tomorrow. A
On 03/08/14 23:43, Allan McRae wrote:
On 03/08/14 22:54, Dave Reisner wrote:
On Sun, Aug 03, 2014 at 06:39:54PM +1000, Allan McRae wrote:
On 03/08/14 01:54, 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:
Can you clarify these bugs for me?
1) The wrong variable is shown for the error which would be thrown when, e.g. pkgname=('foopkg' 'bar@pkg')
What error? This pkgname actually works here...
Sorry, I found this error my static analysis, and then randomly chose a name which turns out to be valid (I recall we made an exception for @). If you have something like pkgname=('foopkg' 'bar^pkg') you'll get the error:
==> ERROR: pkgname contains invalid characters: ''
Whereas, if you use pkgname=('bar^pkg' 'foopkg'), or apply this patch, you'll see:
==> ERROR: pkgname contains invalid characters: '^'
2) The "arch" variable is not sanity checked when the PKGBUILD has package_$pkgname() instead of package().
Do you mean with an override in package_$pkgname()?
Actually, it seems to be the case that arch is never sanity checked at all for a singular package -- has nothing to do with how the package function is named. e.g.:
pkgname=foo arch=('i686' 'x86_64') ...
package_foo() { # or name this package() arch=('invalid')
... }
This should explode, but doesn't, and happily makes a package for x86_64 (since my host is x86_64). After patch:
==> ERROR: foo is not available for the 'x86_64' architecture.
Ah... I see that check_sanity tested if the number of packages > 1 because at that stage we have not tested whether package_foo() or package() is used. And now also see we can override stuff in package(), so the #packages > 1 check was useless.
I'll take a look at the patch itself tomorrow.
Patch looks good. Apart from the removing pkver/pkgrel/epoch override checks. I'd suggest just removing this first (it is a trivial patch, remove it from the override array, adjust get_full_version, and check_sanity), and then adjusting this patch. I also assume this fixes FS#40361. Can you add a note of that in the commit message.
On Mon, Aug 04, 2014 at 03:48:38PM +1000, Allan McRae wrote:
On 03/08/14 23:43, Allan McRae wrote:
On 03/08/14 22:54, Dave Reisner wrote:
On Sun, Aug 03, 2014 at 06:39:54PM +1000, Allan McRae wrote:
On 03/08/14 01:54, 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:
Can you clarify these bugs for me?
1) The wrong variable is shown for the error which would be thrown when, e.g. pkgname=('foopkg' 'bar@pkg')
What error? This pkgname actually works here...
Sorry, I found this error my static analysis, and then randomly chose a name which turns out to be valid (I recall we made an exception for @). If you have something like pkgname=('foopkg' 'bar^pkg') you'll get the error:
==> ERROR: pkgname contains invalid characters: ''
Whereas, if you use pkgname=('bar^pkg' 'foopkg'), or apply this patch, you'll see:
==> ERROR: pkgname contains invalid characters: '^'
2) The "arch" variable is not sanity checked when the PKGBUILD has package_$pkgname() instead of package().
Do you mean with an override in package_$pkgname()?
Actually, it seems to be the case that arch is never sanity checked at all for a singular package -- has nothing to do with how the package function is named. e.g.:
pkgname=foo arch=('i686' 'x86_64') ...
package_foo() { # or name this package() arch=('invalid')
... }
This should explode, but doesn't, and happily makes a package for x86_64 (since my host is x86_64). After patch:
==> ERROR: foo is not available for the 'x86_64' architecture.
Ah... I see that check_sanity tested if the number of packages > 1 because at that stage we have not tested whether package_foo() or package() is used. And now also see we can override stuff in package(), so the #packages > 1 check was useless.
I'll take a look at the patch itself tomorrow.
Patch looks good. Apart from the removing pkver/pkgrel/epoch override checks. I'd suggest just removing this first (it is a trivial patch, remove it from the override array, adjust get_full_version, and check_sanity), and then adjusting this patch.
Will do.
I also assume this fixes FS#40361. Can you add a note of that in the commit message.
Yes it should. I'll update the commit message and rebase my branch.
participants (3)
-
Allan McRae
-
Dave Reisner
-
Dave Reisner