[pacman-dev] [PATCH 1/3] makepkg: use bash 4.4 to localize `set` without explicitly saving/restoring
Signed-off-by: Eli Schwartz eschwartz@archlinux.org --- scripts/makepkg.sh.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 4024f477..bb8332c6 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -418,13 +418,14 @@ prepare_buildenv() { }
run_function_safe() { - local restoretrap restoreset restoreshopt + local restoretrap restoreshopt
# we don't set any special shopts of our own, but we don't want the user to # muck with our environment. restoreshopt=$(shopt -p)
- restoreset=$(shopt -o -p) + # localize sets, sadly this does not work for shopt + local - shopt -o -s errexit errtrace
restoretrap=$(trap -p ERR) @@ -434,7 +435,6 @@ run_function_safe() {
trap - ERR eval "$restoretrap" - eval "$restoreset" eval "$restoreshopt" }
Both run_function and run_function_safe will save and restore `shopt -p` but the former is only called from the latter. It makes sense to save this as part of a "safe" runner, so let's just do it in one place, there where we save and restore everything else too.
Signed-off-by: Eli Schwartz eschwartz@archlinux.org --- scripts/makepkg.sh.in | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index bb8332c6..bb24c633 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -447,9 +447,6 @@ run_function() { msg "$(gettext "Starting %s()...")" "$pkgfunc" cd_safe "$srcdir"
- # save our shell options so pkgfunc() can't override what we need - local shellopts=$(shopt -p) - local ret=0 if (( LOGGING )); then local fullver=$(get_full_version) @@ -479,8 +476,6 @@ run_function() { else "$pkgfunc" fi - # reset our shell options - eval "$shellopts" }
run_prepare() {
On 14/08/18 11:20, Eli Schwartz wrote:
Both run_function and run_function_safe will save and restore `shopt -p` but the former is only called from the latter. It makes sense to save this as part of a "safe" runner, so let's just do it in one place, there where we save and restore everything else too.
Signed-off-by: Eli Schwartz eschwartz@archlinux.org
OK.
A
`run_function_safe pkgver` is evaluated in a subshell and therefore does not abort when it should. Explicitly check the return outside of the subshell and abort if necessary.
Signed-off-by: Eli Schwartz eschwartz@archlinux.org --- scripts/makepkg.sh.in | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index bb24c633..1ab2ea3c 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -188,6 +188,9 @@ enter_fakeroot() { # Re-sources the PKGBUILD afterwards to allow for other variables that use $pkgver update_pkgver() { newpkgver=$(run_function_safe pkgver) + if (( $? != 0 )); then + error_function pkgver + fi if ! check_pkgver "$newpkgver"; then error "$(gettext "pkgver() generated an invalid version: %s")" "$newpkgver" exit $E_PKGBUILD_ERROR
On Mon, 13 Aug 2018 21:20:58 -0400, Eli Schwartz wrote:
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index bb24c633..1ab2ea3c 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -188,6 +188,9 @@ enter_fakeroot() { # Re-sources the PKGBUILD afterwards to allow for other variables that use $pkgver update_pkgver() { newpkgver=$(run_function_safe pkgver)
- if (( $? != 0 )); then
error_function pkgver- fi
Why bring $? in to it, why not:
if ! newpkgver=$(run_function_safe pkgver); then error_function pkgver fi
On 8/13/18 10:13 PM, Luke Shumaker wrote:
On Mon, 13 Aug 2018 21:20:58 -0400, Eli Schwartz wrote:
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index bb24c633..1ab2ea3c 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -188,6 +188,9 @@ enter_fakeroot() { # Re-sources the PKGBUILD afterwards to allow for other variables that use $pkgver update_pkgver() { newpkgver=$(run_function_safe pkgver)
- if (( $? != 0 )); then
error_function pkgver- fi
Why bring $? in to it, why not:
if ! newpkgver=$(run_function_safe pkgver); then error_function pkgver fi
Because that is a complex command and therefore forces errexit to be ignored, hence it will always be successful.
Otherwise I would have simply used ||
Say thank you to http://austingroupbugs.net/view.php?id=537aa
On 14/08/18 11:20, Eli Schwartz wrote:
`run_function_safe pkgver` is evaluated in a subshell and therefore does not abort when it should. Explicitly check the return outside of the subshell and abort if necessary.
Signed-off-by: Eli Schwartz eschwartz@archlinux.org
scripts/makepkg.sh.in | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index bb24c633..1ab2ea3c 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -188,6 +188,9 @@ enter_fakeroot() { # Re-sources the PKGBUILD afterwards to allow for other variables that use $pkgver update_pkgver() { newpkgver=$(run_function_safe pkgver)
- if (( $? != 0 )); then
error_function pkgver- fi if ! check_pkgver "$newpkgver"; then error "$(gettext "pkgver() generated an invalid version: %s")" "$newpkgver" exit $E_PKGBUILD_ERROR
OK.
Also, this looks like another reason to special case running pkgver() which would allow us to cature its output only.
A
On 14/08/18 11:20, Eli Schwartz wrote:
Signed-off-by: Eli Schwartz eschwartz@archlinux.org
scripts/makepkg.sh.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 4024f477..bb8332c6 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -418,13 +418,14 @@ prepare_buildenv() { }
run_function_safe() {
- local restoretrap restoreset restoreshopt
local restoretrap restoreshopt
# we don't set any special shopts of our own, but we don't want the user to # muck with our environment. restoreshopt=$(shopt -p)
- restoreset=$(shopt -o -p)
- # localize sets, sadly this does not work for shopt
- local -
My understanding is this does not quite do the same thing... "local -" only save the single-letter shell options. There are set options that do not have a single letter variants (although they are unlikely to be used...).
On 8/29/18 12:54 AM, Allan McRae wrote:
On 14/08/18 11:20, Eli Schwartz wrote:
Signed-off-by: Eli Schwartz eschwartz@archlinux.org
scripts/makepkg.sh.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 4024f477..bb8332c6 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -418,13 +418,14 @@ prepare_buildenv() { }
run_function_safe() {
- local restoretrap restoreset restoreshopt
local restoretrap restoreshopt
# we don't set any special shopts of our own, but we don't want the user to # muck with our environment. restoreshopt=$(shopt -p)
- restoreset=$(shopt -o -p)
- # localize sets, sadly this does not work for shopt
- local -
My understanding is this does not quite do the same thing... "local -" only save the single-letter shell options. There are set options that do not have a single letter variants (although they are unlikely to be used...).
The manpage does not indicate this; it just says:
the set of shell options is made local to the function in which local is invoked: shell options changed using the set builtin inside the function are restored to their original values when the function returns.
So...
$ testfunc() { shopt -p -o pipefail; local -; set -o pipefail; shopt -p -o pipefail; } $ testfunc; shopt -p -o pipefail set +o pipefail set -o pipefail set +o pipefail
Seems to work okay, fortunately.
On 29/08/18 15:20, Eli Schwartz wrote:
On 8/29/18 12:54 AM, Allan McRae wrote:
On 14/08/18 11:20, Eli Schwartz wrote:
Signed-off-by: Eli Schwartz eschwartz@archlinux.org
scripts/makepkg.sh.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 4024f477..bb8332c6 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -418,13 +418,14 @@ prepare_buildenv() { }
run_function_safe() {
- local restoretrap restoreset restoreshopt
local restoretrap restoreshopt
# we don't set any special shopts of our own, but we don't want the user to # muck with our environment. restoreshopt=$(shopt -p)
- restoreset=$(shopt -o -p)
- # localize sets, sadly this does not work for shopt
- local -
My understanding is this does not quite do the same thing... "local -" only save the single-letter shell options. There are set options that do not have a single letter variants (although they are unlikely to be used...).
The manpage does not indicate this; it just says:
the set of shell options is made local to the function in which local is invoked: shell options changed using the set builtin inside the function are restored to their original values when the function returns.
I was going off the bash-4.4 release notes.
So...
$ testfunc() { shopt -p -o pipefail; local -; set -o pipefail; shopt -p -o pipefail; } $ testfunc; shopt -p -o pipefail set +o pipefail set -o pipefail set +o pipefail
Seems to work okay, fortunately.
Great. Patch is fine then. I'll adjust the comment to "localize set options" as "localize sets" is not clear.
A
On 8/29/18 2:18 AM, Allan McRae wrote:
On 29/08/18 15:20, Eli Schwartz wrote:
On 8/29/18 12:54 AM, Allan McRae wrote:
My understanding is this does not quite do the same thing... "local -" only save the single-letter shell options. There are set options that do not have a single letter variants (although they are unlikely to be used...).
The manpage does not indicate this; it just says:
the set of shell options is made local to the function in which local is invoked: shell options changed using the set builtin inside the function are restored to their original values when the function returns.
I was going off the bash-4.4 release notes.
Hmm, weird. :D
So...
$ testfunc() { shopt -p -o pipefail; local -; set -o pipefail; shopt -p -o pipefail; } $ testfunc; shopt -p -o pipefail set +o pipefail set -o pipefail set +o pipefail
Seems to work okay, fortunately.
Great. Patch is fine then. I'll adjust the comment to "localize set options" as "localize sets" is not clear.
No problem.
participants (3)
-
Allan McRae
-
Eli Schwartz
-
Luke Shumaker