2013/10/3 Dave Reisner :

On Thu, Oct 03, 2013 at 03:49:34PM +0800, Techlive Zheng wrote:

...

--- scripts/makepkg.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f6d884f..1736dc7 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -2809,7 +2809,7 @@ fi

# override settings from extra variables on commandline, if any if (( ${#extra_environment[*]} )); then - export "${extra_environment[@]}" + eval export "${extra_environment[@]}"

NAK. The intention was for the extra args on the commandline to be treated exactly as make treats them. Doing this introduces an (undocumented) layer of indirection that would not be easily discernable at a glance, and probably result in some very unexpected behavior.

Simple example: your change has no regard for whitespace, so something like CFLAGS='-O2 -g' will abort strangely:

/home/noclaf/src/up/pacman/scripts/makepkg: line 2851: export: `-Wnotvalid': not a valid identifier

You'd need to pass CFLAGS='-O2\ -g' which is unexpected and potentially awkward. Consider the case where the value comes from somewhere other than a string literal.

The whitespace concerns could be fixed, but I really don't think this is generally a road we want to go down.

...

fi

# set pkgdir to something "sensible" for (not recommended) use during build() -- 1.8.4

What I want to accomplish with this is that I want to do something like this `makepkg SRCDEST='$BUILDDIR/$pkgbase'` which will store downloaded sources into the directory where they will be built not where the PKGBUILD is. There is no other way I could do this without these patches.

2013/10/3 Dave Reisner :

On Thu, Oct 03, 2013 at 03:49:33PM +0800, Techlive Zheng wrote:

...

This allows for VAR=value and VAR+=value variable declarations in command line to override variables in BUILDSCRIPT. --- scripts/makepkg.sh.in | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1ef2af2..f6d884f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -2712,11 +2712,6 @@ if [[ ! -w $BUILDDIR ]]; then exit 1 fi

-# override settings from extra variables on commandline, if any -if (( ${#extra_environment[*]} )); then - export "${extra_environment[@]}" -fi - PKGDEST=${_PKGDEST:-$PKGDEST} PKGDEST=${PKGDEST:-$startdir} #default to $startdir if undefined if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then @@ -2810,7 +2805,11 @@ if [[ $BUILDDIR = "$startdir" ]]; then else srcdir="$BUILDDIR/$pkgbase/src" pkgdirbase="$BUILDDIR/$pkgbase/pkg" +fi

+# override settings from extra variables on commandline, if any +if (( ${#extra_environment[*]} )); then + export "${extra_environment[@]}"

Doing this is dangerous, as it lets you do things like:

makepkg pkgver=this-is-not-a-valid-version

Allowing a documented feature to override basic sanity checks is not a good idea, imo.

...

fi

# set pkgdir to something "sensible" for (not recommended) use during build() -- 1.8.4

Yes, that is a risk, but people should know what they are doing. My main purpose to move this section here is to use the variables from BUILDSCRIPT in the command line vars assignment in the following patch.

2013/10/3 Allan McRae :

On 03/10/13 23:06, 郑文辉(Techlive Zheng) wrote:

...

2013/10/3 Dave Reisner :

...

On Thu, Oct 03, 2013 at 03:49:33PM +0800, Techlive Zheng wrote:

...

This allows for VAR=value and VAR+=value variable declarations in command line to override variables in BUILDSCRIPT. --- scripts/makepkg.sh.in | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1ef2af2..f6d884f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -2712,11 +2712,6 @@ if [[ ! -w $BUILDDIR ]]; then exit 1 fi

-# override settings from extra variables on commandline, if any -if (( ${#extra_environment[*]} )); then - export "${extra_environment[@]}" -fi - PKGDEST=${_PKGDEST:-$PKGDEST} PKGDEST=${PKGDEST:-$startdir} #default to $startdir if undefined if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then @@ -2810,7 +2805,11 @@ if [[ $BUILDDIR = "$startdir" ]]; then else srcdir="$BUILDDIR/$pkgbase/src" pkgdirbase="$BUILDDIR/$pkgbase/pkg" +fi

+# override settings from extra variables on commandline, if any +if (( ${#extra_environment[*]} )); then + export "${extra_environment[@]}"

Doing this is dangerous, as it lets you do things like:

makepkg pkgver=this-is-not-a-valid-version

Allowing a documented feature to override basic sanity checks is not a good idea, imo.

...

fi

# set pkgdir to something "sensible" for (not recommended) use during build() -- 1.8.4

Yes, that is a risk, but people should know what they are doing. My main purpose to move this section here is to use the variables from BUILDSCRIPT in the command line vars assignment in the following patch.

If people knew what they were doing, we would not need the sanity check in the first place. Also, much of what is check is assumed throughout the rest of makepkg and this would lead to breakages.

You know that you can override SRCDEST et al by using environmental variables already?

You can implement this with a simple wrapper script like:

source PKGBUILD source makepkg.conf

# assuming pkgbase is defined... SRCRDEST=$BUILDDIR/pkgbase PKGDEST=... makepkg "$@"

or something...

Yeah, this was the solution I used before. I named my wrapper as makepkg.sh, but the only problem was that other pacman wrapper like yaourt would not use my wrapper for building, then I started hacking the core of the pacman and it is a pain to keep track pacman's update. Acturally, all I need is to name my wrapper as makepkg too, and use the absolute path for the real makepkg in it, then put my wrapper in a place where appears early in the 'PATH' variable.

2013/10/3 Dave Reisner :

On Thu, Oct 03, 2013 at 09:06:28PM +0800, 郑文辉(Techlive Zheng) wrote:

...

2013/10/3 Dave Reisner :

...

On Thu, Oct 03, 2013 at 03:49:33PM +0800, Techlive Zheng wrote:

...

This allows for VAR=value and VAR+=value variable declarations in command line to override variables in BUILDSCRIPT. --- scripts/makepkg.sh.in | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1ef2af2..f6d884f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -2712,11 +2712,6 @@ if [[ ! -w $BUILDDIR ]]; then exit 1 fi

-# override settings from extra variables on commandline, if any -if (( ${#extra_environment[*]} )); then - export "${extra_environment[@]}" -fi - PKGDEST=${_PKGDEST:-$PKGDEST} PKGDEST=${PKGDEST:-$startdir} #default to $startdir if undefined if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then @@ -2810,7 +2805,11 @@ if [[ $BUILDDIR = "$startdir" ]]; then else srcdir="$BUILDDIR/$pkgbase/src" pkgdirbase="$BUILDDIR/$pkgbase/pkg" +fi

+# override settings from extra variables on commandline, if any +if (( ${#extra_environment[*]} )); then + export "${extra_environment[@]}"

Doing this is dangerous, as it lets you do things like:

makepkg pkgver=this-is-not-a-valid-version

Allowing a documented feature to override basic sanity checks is not a good idea, imo.

...

fi

# set pkgdir to something "sensible" for (not recommended) use during build() -- 1.8.4

Yes, that is a risk, but people should know what they are doing.

And what are the rules? Where have you documented this?

Regardless, I don't think we should require that people have a strong understanding of shell quoting rules and hand escape their values. It's going to make for a really crappy user experience, especially given the failure modes. I realize that this probably isn't going to be a widely used feature, but it should still make sense and have easily understood behavior.

...

My main purpose to move this section here is to use the variables from BUILDSCRIPT in the command line vars assignment in the following patch.

Yes, I surmised that from your other posts. I'd strongly prefer that you find another way. Sorry.

Yeah, I see it now. It is really a personal setup, should not mess up with the core of the pacman. Thanks for all you guys' quick response and help.