[pacman-dev] Comments with `)' are either broken or disallowed
In `makepkg' (that is, in `scripts/makepkg.sh.in'), the following exist: local provides_list=() eval $(awk '/^[[:space:]]*provides=/,/\)/' "$BUILDFILE" | \ sed -e "s/provides=/provides_list+=/" -e "s/#.*//" -e 's/\\$//') ... local backup_list=() eval $(awk '/^[[:space:]]*backup=/,/\)/' "$BUILDFILE" | \ sed -e "s/backup=/backup_list+=/" -e "s/#.*//" -e 's/\\$//') ... local optdepends_list=() eval $(awk '/^[[:space:]]*optdepends=\(/,/\)[[:space:]]*(#.*)?$/' "$BUILDFILE" | \ sed -e "s/optdepends=/optdepends_list+=/" -e "s/#.*//" -e 's/\\$//') ... local known kopt options_list eval $(awk '/^[[:space:]]*options=/,/\)/' "$BUILDFILE" | \ sed -e "s/options=/options_list+=/" -e "s/#.*//" -e 's/\\$//') Perhaps there are more. As you can plainly see, this ruins the ability to include comments that use the `)' character, as in the following: options=( '!strip' '!makeflags' # Apparently, there are issues with concurrency (`-j2', etc.) ) Sincerely, Michael Witten
On 12 May 2014 12:47, Michael Witten <mfwitten@gmail.com> wrote:
As you can plainly see, this ruins the ability to include comments that use the `)' character, as in the following:
From 77482a0064919677e5e948e6c00666d0fffe7486 Mon Sep 17 00:00:00 2001 From: Phillip Smith <fukawi2@gmail.com> Date: Mon, 12 May 2014 14:00:51 +1000 Subject: [PATCH] strip comments early when evaluating
I think this patch may resolve that by stripping comments early. Posting on-list to get feedback. Is there a FS# for this issue? provides/backups/optdepends/options stripping the comments as the first step avoids errors when these arrays contain multiple lines with comments on each line where the comment contains a round-bracket eg: options=( '!strip' '!makeflags' # issues with concurrency (`-j2', etc.) ) --- scripts/makepkg.sh.in | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index d8cdc88..08d19ca 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -2189,8 +2189,9 @@ check_sanity() { fi local provides_list=() - eval $(awk '/^[[:space:]]*provides=/,/\)/' "$BUILDFILE" | \ - sed -e "s/provides=/provides_list+=/" -e "s/#.*//" -e 's/\\$//') + eval $(sed -e "s/[[:space:]]*#.*//" "$BUILDFILE" | \ + awk '/^[[:space:]]*provides=/,/\)/' | \ + sed -e "s/provides=/provides_list+=/" -e 's/\\$//') for i in ${provides_list[@]}; do if [[ $i == *['<>']* ]]; then error "$(gettext "%s array cannot contain comparison (< or >) operators.")" "provides" @@ -2199,8 +2200,9 @@ check_sanity() { done local backup_list=() - eval $(awk '/^[[:space:]]*backup=/,/\)/' "$BUILDFILE" | \ - sed -e "s/backup=/backup_list+=/" -e "s/#.*//" -e 's/\\$//') + eval $(sed -e "s/[[:space:]]*#.*//" "$BUILDFILE" | \ + awk '/^[[:space:]]*backup=/,/\)/' | \ + sed -e "s/backup=/backup_list+=/" -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" @@ -2209,8 +2211,9 @@ check_sanity() { done local optdepends_list=() - eval $(awk '/^[[:space:]]*optdepends=\(/,/\)[[:space:]]*(#.*)?$/' "$BUILDFILE" | \ - sed -e "s/optdepends=/optdepends_list+=/" -e "s/#.*//" -e 's/\\$//') + eval $(sed -e "s/[[:space:]]*#.*//" "$BUILDFILE" | \ + awk '/^[[:space:]]*optdepends=\(/,/\)[[:space:]]*(#.*)?$/' | \ + sed -e "s/optdepends=/optdepends_list+=/" -e 's/\\$//') for i in "${optdepends_list[@]}"; do local pkg=${i%%:[[:space:]]*} # the '-' character _must_ be first or last in the character range @@ -2234,8 +2237,9 @@ check_sanity() { local valid_options=1 local known kopt options_list - eval $(awk '/^[[:space:]]*options=/,/\)/' "$BUILDFILE" | \ - sed -e "s/options=/options_list+=/" -e "s/#.*//" -e 's/\\$//') + eval $(sed -e "s/[[:space:]]*#.*//" "$BUILDFILE" | \ + awk '/^[[:space:]]*options=/,/\)/' | \ + sed -e "s/options=/options_list+=/" -e 's/\\$//') for i in ${options_list[@]}; do known=0 # check if option matches a known option or its inverse -- 1.9.2
On Mon, May 12, 2014 at 10:47 AM, Michael Witten <mfwitten@gmail.com> wrote:
In `makepkg' (that is, in `scripts/makepkg.sh.in'), the following exist:
local provides_list=() eval $(awk '/^[[:space:]]*provides=/,/\)/' "$BUILDFILE" | \ sed -e "s/provides=/provides_list+=/" -e "s/#.*//" -e 's/\\$//') ... local backup_list=() eval $(awk '/^[[:space:]]*backup=/,/\)/' "$BUILDFILE" | \ sed -e "s/backup=/backup_list+=/" -e "s/#.*//" -e 's/\\$//') ... local optdepends_list=() eval $(awk '/^[[:space:]]*optdepends=\(/,/\)[[:space:]]*(#.*)?$/' "$BUILDFILE" | \ sed -e "s/optdepends=/optdepends_list+=/" -e "s/#.*//" -e 's/\\$//') ... local known kopt options_list eval $(awk '/^[[:space:]]*options=/,/\)/' "$BUILDFILE" | \ sed -e "s/options=/options_list+=/" -e "s/#.*//" -e 's/\\$//')
Why are we doing such rudimentary parsing + `eval`? Apart from breaking any extra ")", it also breaks any "#" that does not start a comment, which could happen for example in backup file names, or more likely in the more freestyle optdepends. These are in the check_sanity function, by the time which is run we already source'd the BUILDFILE, correct? Why don't we use the existing variable arrays then?
Perhaps there are more.
As you can plainly see, this ruins the ability to include comments that use the `)' character, as in the following:
options=( '!strip' '!makeflags' # Apparently, there are issues with concurrency (`-j2', etc.) )
Sincerely, Michael Witten
On Mon, May 12, 2014 at 01:06:20PM +0800, lolilolicon wrote:
On Mon, May 12, 2014 at 10:47 AM, Michael Witten <mfwitten@gmail.com> wrote:
In `makepkg' (that is, in `scripts/makepkg.sh.in'), the following exist:
local provides_list=() eval $(awk '/^[[:space:]]*provides=/,/\)/' "$BUILDFILE" | \ sed -e "s/provides=/provides_list+=/" -e "s/#.*//" -e 's/\\$//') ... local backup_list=() eval $(awk '/^[[:space:]]*backup=/,/\)/' "$BUILDFILE" | \ sed -e "s/backup=/backup_list+=/" -e "s/#.*//" -e 's/\\$//') ... local optdepends_list=() eval $(awk '/^[[:space:]]*optdepends=\(/,/\)[[:space:]]*(#.*)?$/' "$BUILDFILE" | \ sed -e "s/optdepends=/optdepends_list+=/" -e "s/#.*//" -e 's/\\$//') ... local known kopt options_list eval $(awk '/^[[:space:]]*options=/,/\)/' "$BUILDFILE" | \ sed -e "s/options=/options_list+=/" -e "s/#.*//" -e 's/\\$//')
Why are we doing such rudimentary parsing + `eval`? Apart from breaking any extra ")", it also breaks any "#" that does not start a comment, which could happen for example in backup file names, or more likely in the more freestyle optdepends.
These are in the check_sanity function, by the time which is run we already source'd the BUILDFILE, correct? Why don't we use the existing variable arrays then?
You're forgetting about package-level overrides, which aren't available in the environment.
Perhaps there are more.
As you can plainly see, this ruins the ability to include comments that use the `)' character, as in the following:
options=( '!strip' '!makeflags' # Apparently, there are issues with concurrency (`-j2', etc.) )
Sincerely, Michael Witten
On Tue, May 13, 2014 at 1:43 AM, Dave Reisner <d@falconindy.com> wrote:
On Mon, May 12, 2014 at 01:06:20PM +0800, lolilolicon wrote:
These are in the check_sanity function, by the time which is run we already source'd the BUILDFILE, correct? Why don't we use the existing variable arrays then?
You're forgetting about package-level overrides, which aren't available in the environment.
Alright, we're cursed, aren't we? Gone are dreams of grace and order, now we must indulge in patch and scratch. Anyway, maybe we can ask bash to pre-formalize the PKGBUILD a little bit so be can parse it more confidently com, by using abination of `set -x` and `typeset`. For example, consider the following troublesome PKGBUILD: foo=(a \ b # ) 'c: )' ) package_one() { foo=(d 'e: 4 #1') } package_two() { foo+=(z) } We can append to the end of it: typeset -f package_one package_two And then if you execute it with: PS4='' bash -x ./PKGBUILD, the output is formalized, with comments stripped too: foo=(a b 'c: )') typeset -f package_one package_two package_one () { foo=(d 'e: 4 #1') } package_two () { foo+=(z) } What do you think?
On Tue, May 13, 2014 at 1:18 PM, lolilolicon <lolilolicon@gmail.com> wrote:
Anyway, maybe we can ask bash to pre-formalize the PKGBUILD a little bit so be can parse it more confidently com, by using abination of `set -x` and `typeset`.
don't know how it got messed up there, should have been "by using a combination of". actually, we can simply use typeset alone too. With `typeset foo` and `typeset package_{one,two}`. etc etc.
On 12/05/14 10:25 PM, lolilolicon wrote:
On Tue, May 13, 2014 at 1:18 PM, lolilolicon <lolilolicon@gmail.com> wrote:
Anyway, maybe we can ask bash to pre-formalize the PKGBUILD a little bit so be can parse it more confidently com, by using abination of `set -x` and `typeset`. don't know how it got messed up there, should have been "by using a combination of".
actually, we can simply use typeset alone too. With `typeset foo` and `typeset package_{one,two}`. etc etc.
I went searching for a way around this and found your thread https://bbs.archlinux.org/viewtopic.php?id=85726. Has someone tried to get "source only variables" accepted upstream in bash?
On Wed, May 14, 2014 at 3:01 AM, Connor Behan <connor.behan@gmail.com> wrote:
On 12/05/14 10:25 PM, lolilolicon wrote:
On Tue, May 13, 2014 at 1:18 PM, lolilolicon <lolilolicon@gmail.com> wrote:
Anyway, maybe we can ask bash to pre-formalize the PKGBUILD a little bit so be can parse it more confidently com, by using abination of `set -x` and `typeset`. don't know how it got messed up there, should have been "by using a combination of".
actually, we can simply use typeset alone too. With `typeset foo` and `typeset package_{one,two}`. etc etc.
I went searching for a way around this and found your thread https://bbs.archlinux.org/viewtopic.php?id=85726. Has someone tried to get "source only variables" accepted upstream in bash?
Wow, that was long ago. I don't think anyone has requested that, but there don't seem to be a case for it either. There probably is no way to separate variable assignments out cleanly from the soup, because variable assignments can be complicated, such as var=$(func), or when flow control such as if-else makes assignments conditional. etc. etc.
On Wed, May 14, 2014 at 1:02 PM, lolilolicon <lolilolicon@gmail.com> wrote:
Wow, that was long ago. I don't think anyone has requested that, but there don't seem to be a case for it either. There probably is no way to separate variable assignments out cleanly from the soup, because variable assignments can be complicated, such as var=$(func), or when flow control such as if-else makes assignments conditional. etc. etc.
By "flow control", I don't mean something like a bladder... The correct term should be "control flow" or "flow of control". *palmface*
On Tue, May 13, 2014 at 01:18:33PM +0800, lolilolicon wrote:
On Tue, May 13, 2014 at 1:43 AM, Dave Reisner <d@falconindy.com> wrote:
On Mon, May 12, 2014 at 01:06:20PM +0800, lolilolicon wrote:
These are in the check_sanity function, by the time which is run we already source'd the BUILDFILE, correct? Why don't we use the existing variable arrays then?
You're forgetting about package-level overrides, which aren't available in the environment.
Alright, we're cursed, aren't we? Gone are dreams of grace and order, now we must indulge in patch and scratch.
Well, I'd like to think there's a slightly better tomorrow out there, but I'm largely biased: https://github.com/falconindy/pkgbuild-introspection Not yet mature enough for makepkg, IMO, but it'll be powering AUR 3.0's enhanced metadata.
Anyway, maybe we can ask bash to pre-formalize the PKGBUILD a little bit so be can parse it more confidently com, by using abination of `set -x` and `typeset`. For example, consider the following troublesome PKGBUILD:
foo=(a \ b # ) 'c: )' )
package_one() { foo=(d 'e: 4 #1') }
package_two() { foo+=(z) }
We can append to the end of it:
typeset -f package_one package_two
Well, no one these days should be using typeset. Always use declare.
And then if you execute it with: PS4='' bash -x ./PKGBUILD, the output is formalized, with comments stripped too:
foo=(a b 'c: )') typeset -f package_one package_two package_one () { foo=(d 'e: 4 #1') } package_two () { foo+=(z) }
What do you think?
Sure, we do this elsewhere in makepkg. I'm not clear on why this can't also apply to check_sanity since, as you point out, we've already sourced the build file by the time this runs. d
On Tue, May 13, 2014 at 8:51 PM, Dave Reisner <d@falconindy.com> wrote:
On Tue, May 13, 2014 at 01:18:33PM +0800, lolilolicon wrote:
On Tue, May 13, 2014 at 1:43 AM, Dave Reisner <d@falconindy.com> wrote:
On Mon, May 12, 2014 at 01:06:20PM +0800, lolilolicon wrote:
These are in the check_sanity function, by the time which is run we already source'd the BUILDFILE, correct? Why don't we use the existing variable arrays then?
You're forgetting about package-level overrides, which aren't available in the environment.
Alright, we're cursed, aren't we? Gone are dreams of grace and order, now we must indulge in patch and scratch.
Well, I'd like to think there's a slightly better tomorrow out there, but I'm largely biased:
Oh nice! Your extract_*_var functions are exactly what we need here in this particular case. OK now, since the wheel is already invented, hope we get it rolling soon!
Not yet mature enough for makepkg, IMO, but it'll be powering AUR 3.0's enhanced metadata.
Since you're on it (the right track) and I trust your judgement, I'm hopeful for a better tomorrow :)
On 12/05/14 12:47, Michael Witten wrote:
In `makepkg' (that is, in `scripts/makepkg.sh.in'), the following exist:
local provides_list=() eval $(awk '/^[[:space:]]*provides=/,/\)/' "$BUILDFILE" | \ sed -e "s/provides=/provides_list+=/" -e "s/#.*//" -e 's/\\$//') ... local backup_list=() eval $(awk '/^[[:space:]]*backup=/,/\)/' "$BUILDFILE" | \ sed -e "s/backup=/backup_list+=/" -e "s/#.*//" -e 's/\\$//') ... local optdepends_list=() eval $(awk '/^[[:space:]]*optdepends=\(/,/\)[[:space:]]*(#.*)?$/' "$BUILDFILE" | \ sed -e "s/optdepends=/optdepends_list+=/" -e "s/#.*//" -e 's/\\$//') ... local known kopt options_list eval $(awk '/^[[:space:]]*options=/,/\)/' "$BUILDFILE" | \ sed -e "s/options=/options_list+=/" -e "s/#.*//" -e 's/\\$//')
Perhaps there are more.
As you can plainly see, this ruins the ability to include comments that use the `)' character, as in the following:
options=( '!strip' '!makeflags' # Apparently, there are issues with concurrency (`-j2', etc.) )
This needs a bug in the bug tracker. Allan
On Wed, 14 May 2014 20:09:15 +1000, Allan McRae wrote:
On 12/05/14 12:47, Michael Witten wrote:
In `makepkg' (that is, in `scripts/makepkg.sh.in'), the following exist:
local provides_list=() eval $(awk '/^[[:space:]]*provides=/,/\)/' "$BUILDFILE" | \ sed -e "s/provides=/provides_list+=/" -e "s/#.*//" -e 's/\\$//') ... local backup_list=() eval $(awk '/^[[:space:]]*backup=/,/\)/' "$BUILDFILE" | \ sed -e "s/backup=/backup_list+=/" -e "s/#.*//" -e 's/\\$//') ... local optdepends_list=() eval $(awk '/^[[:space:]]*optdepends=\(/,/\)[[:space:]]*(#.*)?$/' "$BUILDFILE" | \ sed -e "s/optdepends=/optdepends_list+=/" -e "s/#.*//" -e 's/\\$//') ... local known kopt options_list eval $(awk '/^[[:space:]]*options=/,/\)/' "$BUILDFILE" | \ sed -e "s/options=/options_list+=/" -e "s/#.*//" -e 's/\\$//')
Perhaps there are more.
As you can plainly see, this ruins the ability to include comments that use the `)' character, as in the following:
options=( '!strip' '!makeflags' # Apparently, there are issues with concurrency (`-j2', etc.) )
This needs a bug in the bug tracker.
Allan
participants (6)
-
Allan McRae
-
Connor Behan
-
Dave Reisner
-
lolilolicon
-
Michael Witten
-
Phillip Smith