[pacman-dev] [PATCH] makepkg: bad code rep in bscript parsing
Signed-off-by: Andres P <aepd87@gmail.com> --- scripts/makepkg.sh.in | 75 +++++++++++++++++-------------------------------- 1 files changed, 26 insertions(+), 49 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..00b96a9 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1070,6 +1070,30 @@ create_package() { fi } +parse_buildscript() { + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT") + local file + for file in ${!i}; do + # evaluate any bash variables used + eval file=${file} + + [[ $1 ]] || file=${srclinks}/${pkgbase}/$file + [[ -f $file ]] && return + + if [[ $1 ]]; then + error "$(gettext "${i^} file (%s) does not exist.")" "$file" + return 1 + else + msg2 "$(gettext "Adding $i file (%s)...")" "$file" + ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" + fi + done + done +} + + create_srcpackage() { cd "$startdir" @@ -1107,31 +1131,7 @@ create_srcpackage() { fi done - local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then - local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding install script (%s)...")" "$file" - ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then - local file - for file in ${changelog_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding package changelog (%s)...")" "$file" - ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" - fi - done - fi + parse_buildscript local TAR_OPT case "$SRCEXT" in @@ -1250,30 +1250,7 @@ check_sanity() { fi done - local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then - local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Install scriptlet (%s) does not exist.")" "$file" - return 1 - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then - for file in ${changelog_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Changelog file (%s) does not exist.")" "$file" - return 1 - fi - done - fi + parse_buildscript check local valid_options=1 local opt known kopt -- 1.7.1
On 23/05/10 20:58, Andres P wrote:
Signed-off-by: Andres P<aepd87@gmail.com> ---
-1 from me. The new function does two things that are completely unrelated apart from the regex they use. It makes no sense to combine them. Also the function name does not reflect what it does at all. There is a limit to how much code refactoring is useful. It needs to be balanced by readability.
scripts/makepkg.sh.in | 75 +++++++++++++++++-------------------------------- 1 files changed, 26 insertions(+), 49 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..00b96a9 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1070,6 +1070,30 @@ create_package() { fi }
+parse_buildscript() { + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT") + local file + for file in ${!i}; do + # evaluate any bash variables used + eval file=${file} + + [[ $1 ]] || file=${srclinks}/${pkgbase}/$file + [[ -f $file ]]&& return + + if [[ $1 ]]; then + error "$(gettext "${i^} file (%s) does not exist.")" "$file" + return 1 + else + msg2 "$(gettext "Adding $i file (%s)...")" "$file" + ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" + fi + done + done +} + + create_srcpackage() { cd "$startdir"
@@ -1107,31 +1131,7 @@ create_srcpackage() { fi done
- local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then - local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding install script (%s)...")" "$file" - ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then - local file - for file in ${changelog_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding package changelog (%s)...")" "$file" - ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" - fi - done - fi + parse_buildscript
local TAR_OPT case "$SRCEXT" in @@ -1250,30 +1250,7 @@ check_sanity() { fi done
- local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then - local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Install scriptlet (%s) does not exist.")" "$file" - return 1 - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then - for file in ${changelog_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Changelog file (%s) does not exist.")" "$file" - return 1 - fi - done - fi + parse_buildscript check
local valid_options=1 local opt known kopt
On Sun, May 23, 2010 at 2:20 PM, Allan McRae <allan@archlinux.org> wrote:
On 23/05/10 20:58, Andres P wrote:
Signed-off-by: Andres P<aepd87@gmail.com> ---
-1 from me. The new function does two things that are completely unrelated apart from the regex they use. It makes no sense to combine them. Also the function name does not reflect what it does at all.
There is a limit to how much code refactoring is useful. It needs to be balanced by readability.
There is a double combination in that patch. IMO one of them is fine, the one that combines install and changelog in a for loop. And that combination could be done in the two places, but keeping them separate.
v2: Instead of a monolithic function, split the logic into two for loops. Still less code repetition than master. Signed-off-by: Andres P <aepd87@gmail.com> --- On Sun, May 23, 2010 at 06:43:21PM +0200, Xavier Chantry wrote:
On Sun, May 23, 2010 at 2:20 PM, Allan McRae <allan@archlinux.org> wrote:
-1 from me. The new function does two things that are completely unrelated apart from the regex they use. It makes no sense to combine them. Also the function name does not reflect what it does at all.
There is a limit to how much code refactoring is useful. It needs to be balanced by readability.
There is a double combination in that patch. IMO one of them is fine, the one that combines install and changelog in a for loop. And that combination could be done in the two places, but keeping them separate.
I agree, it's now split In case new fields get added, wether it be for splitdbg or a new zomg feature, it's important to abstract this into a list than can be easily amended. ... scripts/makepkg.sh.in | 53 +++++++++++++----------------------------------- 1 files changed, 15 insertions(+), 38 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..9920aba 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1107,31 +1107,19 @@ create_srcpackage() { fi done - local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then - local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding install script (%s)...")" "$file" - ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT") local file - for file in ${changelog_files[@]}; do + for file in ${!i}; do # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding package changelog (%s)...")" "$file" + eval file='${srclinks}/${pkgbase}/'$file + if [[ ! -f $file ]]; then + msg2 "$(gettext "Adding %s file (%s)...")" "$i" "$file" ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" fi done - fi + done local TAR_OPT case "$SRCEXT" in @@ -1250,30 +1238,19 @@ check_sanity() { fi done - local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT") local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Install scriptlet (%s) does not exist.")" "$file" - return 1 - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then - for file in ${changelog_files[@]}; do + for file in ${!i}; do # evaluate any bash variables used eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Changelog file (%s) does not exist.")" "$file" + if [[ !-f $file ]]; then + error "$(gettext "%s file (%s) does not exist.")" "${i^}" "$file" return 1 fi done - fi + done local valid_options=1 local opt known kopt -- 1.7.1
v2.1: Missed a space in a test clause v2: Instead of a monolithic function, split the logic into two for loops. Still less code repetition than master. Signed-off-by: Andres P <aepd87@gmail.com> --- scripts/makepkg.sh.in | 51 +++++++++++++----------------------------------- 1 files changed, 14 insertions(+), 37 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..4f1b852 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1107,31 +1107,19 @@ create_srcpackage() { fi done - local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then - local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding install script (%s)...")" "$file" - ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT") local file - for file in ${changelog_files[@]}; do + for file in ${!i}; do # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding package changelog (%s)...")" "$file" + eval file='${srclinks}/${pkgbase}/'$file + if [[ ! -f $file ]]; then + msg2 "$(gettext "Adding %s file (%s)...")" "$i" "$file" ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" fi done - fi + done local TAR_OPT case "$SRCEXT" in @@ -1250,30 +1238,19 @@ check_sanity() { fi done - local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT") local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Install scriptlet (%s) does not exist.")" "$file" - return 1 - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then - for file in ${changelog_files[@]}; do + for file in ${!i}; do # evaluate any bash variables used eval file=${file} if [[ ! -f $file ]]; then - error "$(gettext "Changelog file (%s) does not exist.")" "$file" + error "$(gettext "%s file (%s) does not exist.")" "${i^}" "$file" return 1 fi done - fi + done local valid_options=1 local opt known kopt -- 1.7.1
On Sun, May 23, 2010 at 1:25 PM, Andres P <aepd87@gmail.com> wrote:
v2.1: Missed a space in a test clause
v2: Instead of a monolithic function, split the logic into two for loops. Still less code repetition than master.
Signed-off-by: Andres P <aepd87@gmail.com>
See these three dashes right below here? Any comments you stick there won't end up in the git history, which is where 100% of the stuff above is not helpful one month from now. If you wouldn't mind resubmitting this with the iteration history *below* the dashes and something helpful *above*, that would be awesome. Thanks! -Dan P.S. The words "rep" and "bscript" are really confusing to a non-English speaker, let along me (who had to read the commit message a few times). Just make the title short and sweet (e.g. "makepkg: remove code duplication in buildscript parsing") and elaborate on the actual changes in the commit message.
--- scripts/makepkg.sh.in | 51 +++++++++++++----------------------------------- 1 files changed, 14 insertions(+), 37 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..4f1b852 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1107,31 +1107,19 @@ create_srcpackage() { fi done
- local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then - local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding install script (%s)...")" "$file" - ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT") local file - for file in ${changelog_files[@]}; do + for file in ${!i}; do # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding package changelog (%s)...")" "$file" + eval file='${srclinks}/${pkgbase}/'$file + if [[ ! -f $file ]]; then + msg2 "$(gettext "Adding %s file (%s)...")" "$i" "$file" ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" fi done - fi + done
local TAR_OPT case "$SRCEXT" in @@ -1250,30 +1238,19 @@ check_sanity() { fi done
- local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) - if [[ -n $install_files ]]; then + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT") local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Install scriptlet (%s) does not exist.")" "$file" - return 1 - fi - done - fi - - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) - if [[ -n $changelog_files ]]; then - for file in ${changelog_files[@]}; do + for file in ${!i}; do # evaluate any bash variables used eval file=${file} if [[ ! -f $file ]]; then - error "$(gettext "Changelog file (%s) does not exist.")" "$file" + error "$(gettext "%s file (%s) does not exist.")" "${i^}" "$file" return 1 fi done - fi + done
local valid_options=1 local opt known kopt -- 1.7.1
Merges code in two almost identical chunks in create_srcpackage and check_sanity. Also discards the space kept by regex in ae73d75660 and earlier, since the for loop discards it later on. Signed-off-by: Andres P <aepd87@gmail.com> --- v2.2: ~ Remove unnecesarily kept space in changelog|install temp vars (this vars are word splitted) ~ Clean diff against 80f7c1707c v2.1: Missed a space in a test clause v2: Instead of a monolithic function, split the logic into two for loops. Still less code repetition than master. ... scripts/makepkg.sh.in | 51 +++++++++++++----------------------------------- 1 files changed, 14 insertions(+), 37 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8c0da8b..76b6183 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1107,31 +1107,19 @@ create_srcpackage() { fi done - local install_files=( $(sed -n "s/^\([[:space:]]*\)install=/\1/p" "$BUILDSCRIPT") ) - if [[ -n $install_files ]]; then - local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding install script (%s)...")" "$file" - ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" - fi - done - fi - - local changelog_files=( $(sed -n "s/^\([[:space:]]*\)changelog=/\1/p" "$BUILDSCRIPT") ) - if [[ -n $changelog_files ]]; then + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s/^[[:space:]]*$i=//p" "$BUILDSCRIPT") local file - for file in ${changelog_files[@]}; do + for file in ${!i}; do # evaluate any bash variables used - eval file=${file} - if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then - msg2 "$(gettext "Adding package changelog (%s)...")" "$file" + eval file='${srclinks}/${pkgbase}/'${file} + if [[ ! -f $file ]]; then + msg2 "$(gettext "Adding %s file (%s)...")" "$i" "$file" ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/" fi done - fi + done local TAR_OPT case "$SRCEXT" in @@ -1250,30 +1238,19 @@ check_sanity() { fi done - local install_files=( $(sed -n "s/^\([[:space:]]*\)install=/\1/p" "$BUILDSCRIPT") ) - if [[ -n $install_files ]]; then + local i + for i in 'changelog' 'install'; do + local $i=$(sed -n "s/^[[:space:]]*$i=//p" "$BUILDSCRIPT") local file - for file in ${install_files[@]}; do - # evaluate any bash variables used - eval file=${file} - if [[ ! -f $file ]]; then - error "$(gettext "Install scriptlet (%s) does not exist.")" "$file" - return 1 - fi - done - fi - - local changelog_files=( $(sed -n "s/^\([[:space:]]*\)changelog=/\1/p" "$BUILDSCRIPT") ) - if [[ -n $changelog_files ]]; then - for file in ${changelog_files[@]}; do + for file in ${!i}; do # evaluate any bash variables used eval file=${file} if [[ ! -f $file ]]; then - error "$(gettext "Changelog file (%s) does not exist.")" "$file" + error "$(gettext "%s file (%s) does not exist.")" "${i^}" "$file" return 1 fi done - fi + done local valid_options=1 local opt known kopt -- 1.7.1
On 26/05/10 07:02, Andres P wrote:
Merges code in two almost identical chunks in create_srcpackage and check_sanity.
Also discards the space kept by regex in ae73d75660 and earlier, since the for loop discards it later on.
Signed-off-by: Andres P<aepd87@gmail.com> ---
Great. Pushed to my working branch. Allan
participants (4)
-
Allan McRae
-
Andres P
-
Dan McGee
-
Xavier Chantry