[pacman-dev] [PATCH] makepkg - add check for valid options in PKGBUILD
This patch removes the code block in makepkg that checked for depreciated options in a PKGBUILD 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@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..690ab9c 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' 'ccache' 'distcc' 'makeflags' 'force') +valid_options=0 +for opt in ${options[@]}; do + known=1 + for kopt in ${known_options[@]}; do + if [ "${opt}" = "${kopt}" -o "${opt}" = "!${kopt}" ]; then + known=0 + fi + done + if [ $known -eq 1 ]; then + error "$(gettext "Unknown option '%s'")" "$opt" + valid_options=1 + fi +done +if [ $valid_options -eq 1 ]; then + exit 1 +fi + # We need to run devel_update regardless of whether we are in the fakeroot # build process so that if the user runs makepkg --forcever manually, we # 1) output the correct pkgver, and 2) use the correct filename when -- 1.5.5.1
On Mon, May 12, 2008 at 4:35 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
Unknown and depreciated options are upgraded to error conditions.
+known_options=('strip' 'docs' 'libtool' 'emptydirs' 'ccache' 'distcc' 'makeflags' 'force') +valid_options=0 +for opt in ${options[@]}; do + known=1 + for kopt in ${known_options[@]}; do + if [ "${opt}" = "${kopt}" -o "${opt}" = "!${kopt}" ]; then + known=0 + fi + done + if [ $known -eq 1 ]; then + error "$(gettext "Unknown option '%s'")" "$opt" + valid_options=1 + fi +done +if [ $valid_options -eq 1 ]; then + exit 1 +fi +
I see how this can be useful. I still find it a bit disappointing to have to maintain a list of valid options but I don't know.. Btw, if this is for master, you forgot that one: http://projects.archlinux.org/?p=pacman.git;a=commitdiff;h=dae3f9deefdb86f72...
Xavier wrote:
On Mon, May 12, 2008 at 4:35 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
Unknown and depreciated options are upgraded to error conditions.
+known_options=('strip' 'docs' 'libtool' 'emptydirs' 'ccache' 'distcc' 'makeflags' 'force') +valid_options=0 +for opt in ${options[@]}; do + known=1 + for kopt in ${known_options[@]}; do + if [ "${opt}" = "${kopt}" -o "${opt}" = "!${kopt}" ]; then + known=0 + fi + done + if [ $known -eq 1 ]; then + error "$(gettext "Unknown option '%s'")" "$opt" + valid_options=1 + fi +done +if [ $valid_options -eq 1 ]; then + exit 1 +fi +
I see how this can be useful. I still find it a bit disappointing to have to maintain a list of valid options but I don't know..
From experience, if we are going to remove the depreciated options we need to enforce it. There are still 77 packages in extra using 'nolibtool', some of which I am sure were updated recently. 1 still uses keepdocs. Without this, the packages will get updated and these will get overlooked. It is also good for catching spelling mistakes.
Btw, if this is for master, you forgot that one: http://projects.archlinux.org/?p=pacman.git;a=commitdiff;h=dae3f9deefdb86f72...
So I did... I can see how having to maintain the list of options could cause trouble! But I can't think of another way to do this. Updated version of the patch will arrive tomorrow. Allan
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@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') +valid_options=0 +for opt in ${options[@]}; do + known=1 + for kopt in ${known_options[@]}; do + if [ "${opt}" = "${kopt}" -o "${opt}" = "!${kopt}" ]; then + known=0 + fi + done + if [ $known -eq 1 ]; then + error "$(gettext "Unknown option '%s'")" "$opt" + valid_options=1 + fi +done +if [ $valid_options -eq 1 ]; then + exit 1 +fi + # We need to run devel_update regardless of whether we are in the fakeroot # build process so that if the user runs makepkg --forcever manually, we # 1) output the correct pkgver, and 2) use the correct filename when -- 1.5.5.1
On Mon, May 12, 2008 at 8:51 PM, Allan McRae <mcrae_allan@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@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
Dan McGee wrote:
On Mon, May 12, 2008 at 8:51 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
+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.
I had to lookup what the -a flag actually did, but we do need it because known_options is an array. New version of the patch on its way. Allan
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@hotmail.com> --- scripts/makepkg.sh.in | 41 +++++++++++++++++++++-------------------- 1 files changed, 21 insertions(+), 20 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 53d7f98..72d740f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -38,6 +38,8 @@ confdir='@sysconfdir@' startdir="$PWD" srcdir="$startdir/src" pkgdir="$startdir/pkg" +known_options=('strip' 'docs' 'libtool' 'emptydirs' 'zipman' 'ccache' 'distcc' 'makeflags' 'force') +readonly -a known_options # Options ASROOT=0 @@ -182,25 +184,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 +853,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 +1351,25 @@ if [ "$install" -a ! -f "$install" ]; then exit 1 fi +valid_options=0 +for opt in ${options[@]}; do + known=1 + # check if option matches a known option or its inverse + for kopt in ${known_options[@]}; do + if [ "${opt}" = "${kopt}" -o "${opt}" = "!${kopt}" ]; then + known=0 + fi + done + if [ $known -eq 1 ]; then + error "$(gettext "options array contains unknown option '%s'")" "$opt" + valid_options=1 + fi +done +if [ $valid_options -eq 1 ]; then + exit 1 +fi +unset valid_options opt known kopt + # We need to run devel_update regardless of whether we are in the fakeroot # build process so that if the user runs makepkg --forcever manually, we # 1) output the correct pkgver, and 2) use the correct filename when -- 1.5.5.1
On Tue, May 13, 2008 at 12:26 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
+valid_options=0 +for opt in ${options[@]}; do + known=1 + # check if option matches a known option or its inverse + for kopt in ${known_options[@]}; do + if [ "${opt}" = "${kopt}" -o "${opt}" = "!${kopt}" ]; then + known=0 + fi + done + if [ $known -eq 1 ]; then + error "$(gettext "options array contains unknown option '%s'")" "$opt" + valid_options=1 + fi +done +if [ $valid_options -eq 1 ]; then + exit 1 +fi
No big deal, but in my mind, the boolean value should be reversed. known == 1 == true, or known == 0 == false. So inverting known and valid_options make more sense to me :) Other than that, the patch looks fine so I pulled it with that change : http://shining.toofishes.net/gitweb/gitweb.cgi?p=pacman.git;a=commitdiff;h=8...
participants (3)
-
Allan McRae
-
Dan McGee
-
Xavier