[pacman-dev] Comments with `)' are either broken or disallowed

Michael Witten

12 May 2014 12 May '14

10:47 a.m.

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

Show replies by date

Phillip Smith

12 May 12 May

12:05 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On 12 May 2014 12:47, Michael Witten 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 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

lolilolicon

1:06 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On Mon, May 12, 2014 at 10:47 AM, 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/\\$//')

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

Dave Reisner

13 May 13 May

1:43 a.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On Mon, May 12, 2014 at 01:06:20PM +0800, lolilolicon wrote:

...

On Mon, May 12, 2014 at 10:47 AM, 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/\\$//')

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

lolilolicon

1:18 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On Tue, May 13, 2014 at 1:43 AM, Dave Reisner 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?

lolilolicon

1:25 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On Tue, May 13, 2014 at 1:18 PM, lolilolicon 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.

Connor Behan

14 May 14 May

3:01 a.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On 12/05/14 10:25 PM, lolilolicon wrote:

...

On Tue, May 13, 2014 at 1:18 PM, lolilolicon 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?

lolilolicon

1:02 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On Wed, May 14, 2014 at 3:01 AM, Connor Behan wrote:

...

On 12/05/14 10:25 PM, lolilolicon wrote:

...
On Tue, May 13, 2014 at 1:18 PM, lolilolicon 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.

lolilolicon

1:10 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On Wed, May 14, 2014 at 1:02 PM, lolilolicon 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*

Dave Reisner

13 May 13 May

8:51 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On Tue, May 13, 2014 at 01:18:33PM +0800, lolilolicon wrote:

...

On Tue, May 13, 2014 at 1:43 AM, Dave Reisner 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

lolilolicon

9:22 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

On Tue, May 13, 2014 at 8:51 PM, Dave Reisner wrote:

...

On Tue, May 13, 2014 at 01:18:33PM +0800, lolilolicon wrote:

...
On Tue, May 13, 2014 at 1:43 AM, Dave Reisner 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

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 :)

Allan McRae

14 May 14 May

6:09 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

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

Michael Witten

10:49 p.m.

New subject: [pacman-dev] Comments with `)' are either broken or disallowed

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

https://bugs.archlinux.org/task/40361

3643

Age (days ago)

3645

Last active (days ago)

List overview

Download

12 comments

6 participants

participants (6)

Allan McRae
Connor Behan
Dave Reisner
lolilolicon
Michael Witten
Phillip Smith