[pacman-dev] [PATCH] makepkg - add check for valid options in PKGBUILD

[Dan McGee]

On Mon, May 12, 2008 at 8:51 PM, Allan McRae <mcrae_allan at hotmail.com> wrote:
> This patch removes the code block in makepkg that checked for
>  depreciated options in a PKGBUILD and provided a workaround.
>
> Unknown and depreciated options are upgraded to error conditions.
>
>
> Also, removed TODO regarding including install script if exists and
>  $install is unset.  That should never happen.
>
>  Signed-off-by: Allan McRae <mcrae_allan at hotmail.com>
>  ---
>   scripts/makepkg.sh.in |   38 ++++++++++++++++++--------------------
>   1 files changed, 18 insertions(+), 20 deletions(-)
>
>  diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>  index 53d7f98..c81282f 100644
>
>
> --- a/scripts/makepkg.sh.in
>  +++ b/scripts/makepkg.sh.in
>  @@ -182,25 +182,6 @@ check_option() {
>                 return
>         fi
>
>  -       # BEGIN DEPRECATED
>  -       # TODO: This code should be removed in the next release of makepkg.
>  -       local needle=$(echo $1 | tr [:upper:] [:lower:])
>  -       local opt
>  -       for opt in ${options[@]}; do
>  -               opt=$(echo $opt | tr [:upper:] [:lower:])
>  -               if [ "$opt" = "no$needle" ]; then
>  -                       warning "$(gettext "Options beginning with 'no' will be deprecated in the next version of makepkg!")"
>  -                       plain "$(gettext "Please replace 'no' with '!': %s -> %s.")" "no$needle" "!$needle"
>  -                       echo 'n' # Disabled
>  -                       return
>  -               elif [ "$opt" = "keepdocs" -a "$needle" = "docs" ]; then
>  -                       warning "$(gettext "Option 'keepdocs' may not work as intended. Please replace with 'docs'.")"
>  -                       echo 'y' # Enabled
>  -                       return
>  -               fi
>  -       done
>  -       # END DEPRECATED
>  -
>         # fall back to makepkg.conf options
>         ret=$(in_opt_array "$1" ${OPTIONS[@]})
>         if [ "$ret" != '?' ]; then
>  @@ -870,7 +851,6 @@ create_package() {
>         local comp_files=".PKGINFO"
>
>         # check for an install script
>  -       # TODO: should we include ${pkgname}.install if it exists and $install is unset?
>         if [ "$install" != "" ]; then
>                 msg2 "$(gettext "Adding install script...")"
>                 cp "$startdir/$install" .INSTALL
>  @@ -1369,6 +1349,24 @@ if [ "$install" -a ! -f "$install" ]; then
>         exit 1
>   fi
>
>  +known_options=('strip' 'docs' 'libtool' 'emptydirs' 'zipman' 'ccache' 'distcc' 'makeflags' 'force')
Perhaps we should throw this somewhere at the top of the script, since
it is a constant? I'm thinking somewhere right after pkgdir & srcdir.
In additoin, we can probably mark with either readonly or readonly -a
as we don't want any scripts mucking with it.

> +valid_options=0
>  +for opt in ${options[@]}; do
>  +       known=1
>  +       for kopt in ${known_options[@]}; do
I missed the ! in the line below here for a second so I wondered why
you were doing double the work. :)
Maybe add a comment: # check if option matches a known option or its inverse
>  +               if [ "${opt}" = "${kopt}" -o "${opt}" = "!${kopt}" ]; then
>  +                       known=0
>  +               fi
>  +       done
>  +       if [ $known -eq 1 ]; then
>  +               error "$(gettext "Unknown option '%s'")" "$opt"
Just so it is clear it is coming from the options array, and to match
the other error messages (where they state the field name first),
maybe this?
options array contains unknown option '%s'
>  +               valid_options=1
>  +       fi
>  +done
>  +if [ $valid_options -eq 1 ]; then
>  +       exit 1
>  +fi
>  +
Add an unset for valid_options, opt, known, and kopt here once we are
done with them- it helps keep our variables as local as possible.
unset valid_options opt known kopt

Otherwise this looks great, thanks for actually reading the TODO
markers in makepkg.

-Dan