[pacman-dev] [PATCH] Fix the implementation of lint_variable.
The old implementation had both false negatives and false positives. Because of the -w option to grep, it didn't detect lines like: license=bad Additionally, it did detect irrelevant lines like this: --with-arch=${_arch} The latter is a false positive which made it difficult to build valid packages. Signed-off-by: David Grayson <davidegrayson@gmail.com> --- scripts/libmakepkg/lint_pkgbuild/variable.sh.in | 26 ++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 1daac26..eee6b54 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -28,6 +28,25 @@ source "$LIBRARY/util/message.sh" lint_pkgbuild_functions+=('lint_variable') +lint_var_defined_as_non_array() { + if [[ "${!1+x}" == "" ]]; then + return 1 # not defined + fi + if [[ "$(declare -p $1)" == "declare -a "* ]]; then + return 1 # defined as an array + fi + return 0 +} + +lint_var_defined_as_array() { + if [[ "${!1+x}" == "" ]]; then + return 1 # not defined + fi + if [[ "$(declare -p $1)" == "declare -a "* ]]; then + return 0 # defined as an array + fi + return 1 +} lint_variable() { # TODO: refactor - similar arrays are used elsewhere @@ -39,17 +58,18 @@ lint_variable() { local i for i in ${array[@]}; do - if grep -w -q -e "$i=[^(]" -e "$i+=[^(]" "$BUILDSCRIPT"; then + if lint_var_defined_as_non_array $i; then error "$(gettext "%s should be an array")" "$i" ret=1 fi done + local a for a in ${arch[@]}; do [[ $a == "any" ]] && continue for i in ${arch_array[@]}; do - if grep -w -q -e "$i_$a=[^(]" -e "$i_$a+=[^(]" "$BUILDSCRIPT"; then + if lint_var_defined_as_non_array $i; then error "$(gettext "%s_%s should be an array")" "$i" "$a" ret=1 fi @@ -57,7 +77,7 @@ lint_variable() { done for i in ${string[@]}; do - if grep -w -q -e "$i=(" -e "$i+=(" "$BUILDSCRIPT"; then + if lint_var_defined_as_array $i; then error "$(gettext "%s should not be an array")" "$i" ret=1 fi -- 2.6.2
On 12/11/15 18:37, David Grayson wrote:
The old implementation had both false negatives and false positives.
Because of the -w option to grep, it didn't detect lines like:
license=bad
Additionally, it did detect irrelevant lines like this:
--with-arch=${_arch}
The latter is a false positive which made it difficult to build valid packages.
This does not catch bad variables in the package() arrays. Q
On 12/11/15 19:18, Allan McRae wrote:
On 12/11/15 18:37, David Grayson wrote:
The old implementation had both false negatives and false positives.
Because of the -w option to grep, it didn't detect lines like:
license=bad
Additionally, it did detect irrelevant lines like this:
--with-arch=${_arch}
The latter is a false positive which made it difficult to build valid packages.
This does not catch bad variables in the package() arrays.
Try this: diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 1daac26..412da25 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -39,7 +39,7 @@ lint_variable() { local i for i in ${array[@]}; do - if grep -w -q -e "$i=[^(]" -e "$i+=[^(]" "$BUILDSCRIPT"; then + if grep -q -e "^[[:space:]]*$i=[^(]" -e "^[[:space:]]*$i+=[^(]" "$BUILDSCRIPT"; then error "$(gettext "%s should be an array")" "$i" ret=1 fi @@ -49,7 +49,7 @@ lint_variable() { [[ $a == "any" ]] && continue for i in ${arch_array[@]}; do - if grep -w -q -e "$i_$a=[^(]" -e "$i_$a+=[^(]" "$BUILDSCRIPT"; then + if grep -q -e "^[[:space:]]*$i_$a=[^(]" -e "^[[:space:]]*$i_$a+=[^(]" "$BUILDSCRIPT"; then error "$(gettext "%s_%s should be an array")" "$i" "$a" ret=1 fi @@ -57,7 +57,7 @@ lint_variable() { done for i in ${string[@]}; do - if grep -w -q -e "$i=(" -e "$i+=(" "$BUILDSCRIPT"; then + if grep -q -e "^[[:space:]]*$i=(" -e "^^[[:space:]]*$i+=(" "$BUILDSCRIPT"; then error "$(gettext "%s should not be an array")" "$i" ret=1 fi
On Thu, Nov 12, 2015 at 1:28 AM, Allan McRae <allan@archlinux.org> wrote:
This does not catch bad variables in the package() arrays.
Thank you for the feedback, Allan! I forgot that variables can be redefined in the package() function and could be defined to bad types at that time. How about we use my original patch but add a call to lint_variable right after package (or similar) is called? (Also I should fix it to use tabs as indentation.) The downside is that someone might build a giant package and then later find out that it fails linting at the very last step, but I think they could use "pacman --repackage" if they want to save time, and I think they would also forgive you for not catching errors in code that has not executed yet. If that sounds good to you, I will work on a new patch that does that.
+ if grep -q -e "^[[:space:]]*$i=[^(]" -e "^[[:space:]]*$i+=[^(]" "$BUILDSCRIPT"; then
Your patch violate's pacman's 80-character line length convention, and it got wrapped by your mail client. I tested your new patch, and it does fix the bugs I was complaining about in my original message, but there are still false positives and false negatives. False positives: make \ arch=${_arch} False negative: eval lic""ense=bad Of course, these are contrived cases that I made up. But if we had a way to download all of Arch Linux's PKGBUILDs we might find some real issues. Since bash is a complex language, I think that attempting to do static analysis of it with a few regular expressions will always have problems. --David
On 13/11/15 03:12, David Grayson wrote:
On Thu, Nov 12, 2015 at 1:28 AM, Allan McRae <allan@archlinux.org> wrote:
This does not catch bad variables in the package() arrays.
Thank you for the feedback, Allan! I forgot that variables can be redefined in the package() function and could be defined to bad types at that time. How about we use my original patch but add a call to lint_variable right after package (or similar) is called? (Also I should fix it to use tabs as indentation.) The downside is that someone might build a giant package and then later find out that it fails linting at the very last step, but I think they could use "pacman --repackage" if they want to save time, and I think they would also forgive you for not catching errors in code that has not executed yet.
If that sounds good to you, I will work on a new patch that does that.
Nope - erroring out during package() is a bad idea. These errors need to be detected at the start. Note there is code for extracting features from the PKGBUILD used to make .SRCINFO files already there. It has issues when arrays are not arrays etc, which is why I added this check. Anyway, maybe we can utilize that instead.
+ if grep -q -e "^[[:space:]]*$i=[^(]" -e "^[[:space:]]*$i+=[^(]" "$BUILDSCRIPT"; then
Your patch violate's pacman's 80-character line length convention, and it got wrapped by your mail client. I tested your new patch, and it does fix the bugs I was complaining about in my original message, but there are still false positives and false negatives.
False positives:
make \ arch=${_arch}
False negative:
eval lic""ense=bad
Of course, these are contrived cases that I made up.
I'm sure that last one is genuine! I'm not too worried about the false negative, but any false positive is an issue. A
On 13/11/15 11:33, Allan McRae wrote:
On 13/11/15 03:12, David Grayson wrote:
On Thu, Nov 12, 2015 at 1:28 AM, Allan McRae <allan@archlinux.org> wrote:
This does not catch bad variables in the package() arrays.
Thank you for the feedback, Allan! I forgot that variables can be redefined in the package() function and could be defined to bad types at that time. How about we use my original patch but add a call to lint_variable right after package (or similar) is called? (Also I should fix it to use tabs as indentation.) The downside is that someone might build a giant package and then later find out that it fails linting at the very last step, but I think they could use "pacman --repackage" if they want to save time, and I think they would also forgive you for not catching errors in code that has not executed yet.
If that sounds good to you, I will work on a new patch that does that.
Nope - erroring out during package() is a bad idea. These errors need to be detected at the start.
Note there is code for extracting features from the PKGBUILD used to make .SRCINFO files already there. It has issues when arrays are not arrays etc, which is why I added this check. Anyway, maybe we can utilize that instead.
+ if grep -q -e "^[[:space:]]*$i=[^(]" -e "^[[:space:]]*$i+=[^(]" "$BUILDSCRIPT"; then
Your patch violate's pacman's 80-character line length convention, and it got wrapped by your mail client. I tested your new patch, and it does fix the bugs I was complaining about in my original message, but there are still false positives and false negatives.
False positives:
make \ arch=${_arch}
False negative:
eval lic""ense=bad
Of course, these are contrived cases that I made up.
I'm sure that last one is genuine! I'm not too worried about the false negative, but any false positive is an issue.
Try the new patches I have just submitted (or use my patchqueue branch). They should be very robust to false positives, and only make false negatives in silly cases... Allan
participants (2)
-
Allan McRae
-
David Grayson