[arch-projects] [devtools] [PATCH 0/7] Backports from Parabola v20180103
From: Luke Shumaker <lukeshu@parabola.nu> These are the commits in Parabola's developer tools v20180103 that are not in upstream devtools that I think are upstreamable. There's no real theme to them. Some of theme have been seen on this mailing list before. (I appologize that I waited 2 weeks to send this; I didn't send it right away, then I forgot.) Eli Schwartz (2): makechrootpkg: Fix unconditionally running namcap makechrootpkg: Fix anti-pattern when checking for enabled features Luke Shumaker (5): arch-nspawn: make sure that makepkg.conf is always parsed as text arch-nspawn: Remove pointless $(echo ...) subshell lib/common.sh: Adjust to work properly with `set -u` makechrootpkg: Adjust to work properly with `set -e` makechrootpkg: Put "keyserver-options auto-key-retrieve" in gpg.conf arch-nspawn.in | 4 ++-- lib/common.sh | 4 +++- makechrootpkg.in | 9 +++++---- 3 files changed, 10 insertions(+), 7 deletions(-) -- Happy hacking, ~ Luke Shumaker
From: Eli Schwartz <eschwartz@archlinux.org> Fixes regression in 2fd5931a8c67289a8a4acd327b3ce99a5d64c8c7 $run_namcap will always be set to "" `if $not_a_var; then ...; fi` is always truthful when $not_a_var is unset or equal to "" and the `then` clause will always be run. I'm not sure why global state variables need to be cloned locally for their sole explicit purpose. But for now this patch implements the minimum necessary work to properly pass the "do I want namcap" variable into prepare_chroot() according to the current logic flow. Note that I have still not thorougly tested makechrootpkg. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> Reviewed-by: Luke Shumaker <lukeshu@parabola.nu> --- makechrootpkg.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 511e519..02c91bc 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -409,7 +409,7 @@ main() { download_sources "$copydir" "$makepkg_user" - prepare_chroot "$copydir" "$USER_HOME" "$keepbuilddir" + prepare_chroot "$copydir" "$USER_HOME" "$keepbuilddir" "$run_namcap" if arch-nspawn "$copydir" \ --bind="$PWD:/startdir" \ -- 2.15.1
From: Eli Schwartz <eschwartz@archlinux.org> Don't use error-prone logic e.g. foo=true; if $foo ... This completely fails to act as expected when the variable is unset because of unrelated bugs. While this merely causes the default behavior to be "false" rather than "true" in such cases, it is better to fail to enable explicitly requested behavior (which will be noticed by the user) than to simply upgrade to this behavior for free (which may not seem to have any obvious cause). Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> Reviewed-by: Luke Shumaker <lukeshu@parabola.nu> --- makechrootpkg.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 02c91bc..c7fe076 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -165,7 +165,7 @@ prepare_chroot() { local keepbuilddir=$3 local run_namcap=$4 - $keepbuilddir || rm -rf "$copydir/build" + [[ $keepbuilddir = true ]] || rm -rf "$copydir/build" local builduser_uid builduser_gid builduser_uid="${SUDO_UID:-$UID}" @@ -208,7 +208,7 @@ EOF declare -p SOURCE_DATE_EPOCH 2>/dev/null printf '_chrootbuild "$@" || exit\n' - if $run_namcap; then + if [[ $run_namcap = true ]]; then declare -f _chrootnamcap printf '_chrootnamcap || exit\n' fi -- 2.15.1
From: Luke Shumaker <lukeshu@parabola.nu> https://lists.parabola.nu/pipermail/dev/2017-June/005576.html --- arch-nspawn.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch-nspawn.in b/arch-nspawn.in index c55f498..3949ee1 100644 --- a/arch-nspawn.in +++ b/arch-nspawn.in @@ -107,7 +107,7 @@ fi build_mount_args copy_hostconf -eval "$(grep '^CARCH=' "$working_dir/etc/makepkg.conf")" +eval "$(grep -a '^CARCH=' "$working_dir/etc/makepkg.conf")" [[ -z $nosetarch ]] || unset CARCH -- 2.15.1
From: Luke Shumaker <lukeshu@parabola.nu> --- arch-nspawn.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch-nspawn.in b/arch-nspawn.in index 3949ee1..7a7a274 100644 --- a/arch-nspawn.in +++ b/arch-nspawn.in @@ -91,7 +91,7 @@ copy_hostconf () { cp -T "$file" "$working_dir$file" done - sed -r "s|^#?\\s*CacheDir.+|CacheDir = $(echo -n "${cache_dirs[@]}")|g" -i "$working_dir/etc/pacman.conf" + sed -r "s|^#?\\s*CacheDir.+|CacheDir = ${cache_dirs[*]}|g" -i "$working_dir/etc/pacman.conf" } # }}} -- 2.15.1
From: Luke Shumaker <lukeshu@parabola.nu> Support for working with `set -u` was broken by 94160d6. Egg on my face; I'm the one who wants `set -u` support, and I'm the author of that commit! libmakepkg does not work with `set -u`; but mostly because of the include guards! So we just need to temporarily disable `set -u` (nounset) while loading libmakepkg. Instead of introducing a new variable, just store the initial nounset status in _INCLUDE_COMMON_SH; rather than a useless fixed-string "true". While we're at it, disable POSIX-mode (just in case we're running as "sh" instead of "bash"), since libmakepkg uses bash-isms that won't parse in POSIX mode. --- lib/common.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/common.sh b/lib/common.sh index a3c2ec2..821f8df 100644 --- a/lib/common.sh +++ b/lib/common.sh @@ -4,10 +4,12 @@ # License: Unspecified [[ -z ${_INCLUDE_COMMON_SH:-} ]] || return 0 -_INCLUDE_COMMON_SH=true +_INCLUDE_COMMON_SH="$(set +o|grep nounset)" +set +u +o posix # shellcheck disable=1091 . /usr/share/makepkg/util.sh +$_INCLUDE_COMMON_SH # Avoid any encoding problems export LANG=C -- 2.15.1
From: Luke Shumaker <lukeshu@parabola.nu> This worked properly until eab5aba. --- makechrootpkg.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index c7fe076..a6c54cc 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -205,7 +205,7 @@ EOF { printf '#!/bin/bash\n' declare -f _chrootbuild - declare -p SOURCE_DATE_EPOCH 2>/dev/null + declare -p SOURCE_DATE_EPOCH 2>/dev/null || true printf '_chrootbuild "$@" || exit\n' if [[ $run_namcap = true ]]; then -- 2.15.1
From: Luke Shumaker <lukeshu@parabola.nu> This allows signature verification by `makepkg --verifysource`, `git verify-tag`, and such without requiring the user to manually retrieve the keys first. This is based off of devtools32 commit 009695b (2017-06-27) by Erich Eckner <git@eckner.net>. There are 2 differences from that commit: - In this version, gpg.conf is owned by builduser, not by root - In this version, we don't keep appending duplicate lines if we re-use a chroot --- makechrootpkg.in | 1 + 1 file changed, 1 insertion(+) diff --git a/makechrootpkg.in b/makechrootpkg.in index a6c54cc..d2a0477 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -186,6 +186,7 @@ prepare_chroot() { [[ -r $USER_HOME/$x ]] || continue $install -m 644 "$USER_HOME/$x" "$copydir/build/$x" done + $install -m644 /dev/stdin "$copydir/build/.gnupg/gpg.conf" <<<'keyserver-options auto-key-retrieve' sed -e '/^MAKEFLAGS=/d' -e '/^PACKAGER=/d' -i "$copydir/etc/makepkg.conf" for x in BUILDDIR=/build PKGDEST=/pkgdest SRCPKGDEST=/srcpkgdest SRCDEST=/srcdest LOGDEST=/logdest \ -- 2.15.1
On 01/15/2018 11:57 AM, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
https://lists.parabola.nu/pipermail/dev/2017-June/005576.html --- arch-nspawn.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch-nspawn.in b/arch-nspawn.in index c55f498..3949ee1 100644 --- a/arch-nspawn.in +++ b/arch-nspawn.in @@ -107,7 +107,7 @@ fi build_mount_args copy_hostconf
-eval "$(grep '^CARCH=' "$working_dir/etc/makepkg.conf")" +eval "$(grep -a '^CARCH=' "$working_dir/etc/makepkg.conf")"
[[ -z $nosetarch ]] || unset CARCH
I'm curious, in what situation were you having grep parse makepkg.conf as not text? I also wonder whether we should source makepkg.conf in a subshell and printf the relevant variables... -- Eli Schwartz Bug Wrangler and Trusted User
On 01/15/2018 11:57 AM, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
libmakepkg does not work with `set -u`; but mostly because of the include guards! So we just need to temporarily disable `set -u` (nounset) while loading libmakepkg. Instead of introducing a new variable, just store the initial nounset status in _INCLUDE_COMMON_SH; rather than a useless fixed-string "true".
Would it make sense to fix this in libmakepkg instead? devtools are not the only project that would reuse libmakepkg components. Also, working towards having makepkg be able to use set -u could be interesting. -- Eli Schwartz Bug Wrangler and Trusted User
On 01/15/2018 11:57 AM, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
This allows signature verification by `makepkg --verifysource`, `git verify-tag`, and such without requiring the user to manually retrieve the keys first.
This is based off of devtools32 commit 009695b (2017-06-27) by Erich Eckner <git@eckner.net>. There are 2 differences from that commit: - In this version, gpg.conf is owned by builduser, not by root - In this version, we don't keep appending duplicate lines if we re-use a chroot
We use --skipinteg inside the chroot anyway, since 75fdff1811a0487f82c75b2e260da905102b4eea -- but this reminds me I need to submit my patch to disable copying of the keyring altogether. -- Eli Schwartz Bug Wrangler and Trusted User
On Mon, 15 Jan 2018 12:11:29 -0500, Eli Schwartz wrote:
On 01/15/2018 11:57 AM, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
https://lists.parabola.nu/pipermail/dev/2017-June/005576.html --- arch-nspawn.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch-nspawn.in b/arch-nspawn.in index c55f498..3949ee1 100644 --- a/arch-nspawn.in +++ b/arch-nspawn.in @@ -107,7 +107,7 @@ fi build_mount_args copy_hostconf
-eval "$(grep '^CARCH=' "$working_dir/etc/makepkg.conf")" +eval "$(grep -a '^CARCH=' "$working_dir/etc/makepkg.conf")"
[[ -z $nosetarch ]] || unset CARCH
I'm curious, in what situation were you having grep parse makepkg.conf as not text?
Some unicode in a comment near the top was tripping it up (this was on the box of a Spanish-speaking developer).
I also wonder whether we should source makepkg.conf in a subshell and printf the relevant variables...
I have no problem with that. -- Happy hacking, ~ Luke Shumaker
On Mon, 15 Jan 2018 12:25:22 -0500, Eli Schwartz via arch-projects wrote:
[1 Re: [arch-projects] [devtools] [PATCH 5/7] lib/common.sh: Adjust to work properly with `set -u` <multipart/mixed (7bit)>] [1.1 <text/plain; utf-8 (quoted-printable)>] On 01/15/2018 11:57 AM, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
libmakepkg does not work with `set -u`; but mostly because of the include guards! So we just need to temporarily disable `set -u` (nounset) while loading libmakepkg. Instead of introducing a new variable, just store the initial nounset status in _INCLUDE_COMMON_SH; rather than a useless fixed-string "true".
Would it make sense to fix this in libmakepkg instead? devtools are not the only project that would reuse libmakepkg components. Also, working towards having makepkg be able to use set -u could be interesting.
Long-term: It absolutely would make sense. But libmakepkg has a lot slower release schedule than devtools/libretools, and I needed to get a release out :) libretools has a few consumers of common.sh that `set -u`; the switch to libmakepkg would have broken them otherwise. -- Happy hacking, ~ Luke Shumaker
On 2018-01-15 18:11, Eli Schwartz via arch-projects wrote:
On 01/15/2018 11:57 AM, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
https://lists.parabola.nu/pipermail/dev/2017-June/005576.html --- arch-nspawn.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch-nspawn.in b/arch-nspawn.in index c55f498..3949ee1 100644 --- a/arch-nspawn.in +++ b/arch-nspawn.in @@ -107,7 +107,7 @@ fi build_mount_args copy_hostconf
-eval "$(grep '^CARCH=' "$working_dir/etc/makepkg.conf")" +eval "$(grep -a '^CARCH=' "$working_dir/etc/makepkg.conf")"
[[ -z $nosetarch ]] || unset CARCH
I'm curious, in what situation were you having grep parse makepkg.conf as not text?
I also wonder whether we should source makepkg.conf in a subshell and printf the relevant variables...
Used to happen with my "ł" in ~/.makepkg.conf, so sounds generally useful.
On 2018-01-15 17:57, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
These are the commits in Parabola's developer tools v20180103 that are not in upstream devtools that I think are upstreamable. There's no real theme to them. Some of theme have been seen on this mailing list before.
(I appologize that I waited 2 weeks to send this; I didn't send it right away, then I forgot.)
Eli Schwartz (2): makechrootpkg: Fix unconditionally running namcap makechrootpkg: Fix anti-pattern when checking for enabled features
Luke Shumaker (5): arch-nspawn: make sure that makepkg.conf is always parsed as text arch-nspawn: Remove pointless $(echo ...) subshell lib/common.sh: Adjust to work properly with `set -u` makechrootpkg: Adjust to work properly with `set -e` makechrootpkg: Put "keyserver-options auto-key-retrieve" in gpg.conf
arch-nspawn.in | 4 ++-- lib/common.sh | 4 +++- makechrootpkg.in | 9 +++++---- 3 files changed, 10 insertions(+), 7 deletions(-)
LGTM. I will wait few days more if someone else would like to review it and then merge it. Bartłomiej
On 01/20/2018 10:15 AM, Bartłomiej Piotrowski via arch-projects wrote:
On 2018-01-15 17:57, Luke Shumaker wrote:
From: Luke Shumaker <lukeshu@parabola.nu>
These are the commits in Parabola's developer tools v20180103 that are not in upstream devtools that I think are upstreamable. There's no real theme to them. Some of theme have been seen on this mailing list before.
(I appologize that I waited 2 weeks to send this; I didn't send it right away, then I forgot.)
Eli Schwartz (2): makechrootpkg: Fix unconditionally running namcap makechrootpkg: Fix anti-pattern when checking for enabled features
Luke Shumaker (5): arch-nspawn: make sure that makepkg.conf is always parsed as text arch-nspawn: Remove pointless $(echo ...) subshell lib/common.sh: Adjust to work properly with `set -u` makechrootpkg: Adjust to work properly with `set -e` makechrootpkg: Put "keyserver-options auto-key-retrieve" in gpg.conf
arch-nspawn.in | 4 ++-- lib/common.sh | 4 +++- makechrootpkg.in | 9 +++++---- 3 files changed, 10 insertions(+), 7 deletions(-)
LGTM. I will wait few days more if someone else would like to review it and then merge it.
Does that include the gpg.conf patch? Because as I said, I'd rather we just got rid of copying ~/.gnupg at all. ;) Also I'm obviously biased in favor of my still-outstanding patches from this patchset. ;) https://patchwork.archlinux.org/project/arch-projects/list/?series=44 -- Eli Schwartz Bug Wrangler and Trusted User
participants (3)
-
Bartłomiej Piotrowski
-
Eli Schwartz
-
Luke Shumaker