[pacman-dev] [PATCH] makepkg: Don't double-layer distcc on ccache
From: Matti Niemenmaa <matti.niemenmaa+git@iki.fi> buildenv is set once for build() and a second time for package(). When using both distcc and ccache, this lead to CCACHE_PREFIX="distcc distcc" in package(), which breaks PKGBUILDs that execute the compiler in package() because distcc complains: distcc[383041] (main) CRITICAL! distcc seems to have invoked itself recursively! Avoid causing this error by only adding "distcc" to CCACHE_PREFIX if it's not yet there. Signed-off-by: Matti Niemenmaa <matti.niemenmaa+git@iki.fi> --- scripts/libmakepkg/buildenv/compiler.sh.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) ncurses is an example of a package that runs into this problem. Setting buildenv twice was done on purpose in commit 02a0bf550a22e199f48537b7eee87361b112e8a0, but at a glance I'm not sure whether it provides any benefit. All the meaningful changes are to environment variables that are exported from the parent process anyway, so all the changes are either repeated or even duplicated (like in PATH, or this CCACHE_PREFIX issue). Maybe it would be better to simply remove the prepare_buildenv call from the INFAKEROOT case in makepkg.sh.in? diff --git a/scripts/libmakepkg/buildenv/compiler.sh.in b/scripts/libmakepkg/buildenv/compiler.sh.in index 69f58a29..c93c77b4 100644 --- a/scripts/libmakepkg/buildenv/compiler.sh.in +++ b/scripts/libmakepkg/buildenv/compiler.sh.in @@ -44,7 +44,9 @@ buildenv_ccache() { buildenv_distcc() { if check_buildoption "distcc" "y"; then if (( using_ccache )); then - export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + if ! [[ "$CCACHE_PREFIX" =~ (^| )distcc($| ) ]]; then + export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + fi export CCACHE_BASEDIR="$srcdir" elif [[ -d /usr/lib/distcc/bin ]]; then export PATH="/usr/lib/distcc/bin:$PATH" -- 2.24.1
On 1/7/20 2:51 AM, Matti Niemenmaa wrote:
From: Matti Niemenmaa <matti.niemenmaa+git@iki.fi>
buildenv is set once for build() and a second time for package(). When using both distcc and ccache, this lead to CCACHE_PREFIX="distcc distcc" in package(), which breaks PKGBUILDs that execute the compiler in package() because distcc complains:
distcc[383041] (main) CRITICAL! distcc seems to have invoked itself recursively!
Avoid causing this error by only adding "distcc" to CCACHE_PREFIX if it's not yet there.
Signed-off-by: Matti Niemenmaa <matti.niemenmaa+git@iki.fi> --- scripts/libmakepkg/buildenv/compiler.sh.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ncurses is an example of a package that runs into this problem.
Setting buildenv twice was done on purpose in commit 02a0bf550a22e199f48537b7eee87361b112e8a0, but at a glance I'm not sure whether it provides any benefit. All the meaningful changes are to environment variables that are exported from the parent process anyway, so all the changes are either repeated or even duplicated (like in PATH, or this CCACHE_PREFIX issue). Maybe it would be better to simply remove the prepare_buildenv call from the INFAKEROOT case in makepkg.sh.in?
makepkg --repackage will skip the build() step, and some package() functions actually result in artifacts being rebuilt (not all build systems handle this properly). While it's true we run prepare_buildenv() before the fakeroot either way, so exported variables are still exported, makepkg.conf will often (usually?) reset them, and we need to specially handle DEBUG_*FLAGS as well as -fdebug-prefix-map. The other major problem I see is that we currently have buildenv/buildflags.sh.in and buildenv/makeflags.sh.in which actually unset variables that may be restored during fakeroot due to reparsing makepkg.conf package() functions, even when --repackage is *not* used, should still respect MAKEFLAGS when running make install, even if you discount needing to have consistent CFLAGS when running a dirty package() function.
diff --git a/scripts/libmakepkg/buildenv/compiler.sh.in b/scripts/libmakepkg/buildenv/compiler.sh.in index 69f58a29..c93c77b4 100644 --- a/scripts/libmakepkg/buildenv/compiler.sh.in +++ b/scripts/libmakepkg/buildenv/compiler.sh.in @@ -44,7 +44,9 @@ buildenv_ccache() { buildenv_distcc() { if check_buildoption "distcc" "y"; then if (( using_ccache )); then - export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + if ! [[ "$CCACHE_PREFIX" =~ (^| )distcc($| ) ]]; then + export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + fi export CCACHE_BASEDIR="$srcdir" elif [[ -d /usr/lib/distcc/bin ]]; then export PATH="/usr/lib/distcc/bin:$PATH"
-- Eli Schwartz Bug Wrangler and Trusted User
On Wed, Jan 08, 2020 at 02:15:45AM -0500, Eli Schwartz wrote:
On 1/7/20 2:51 AM, Matti Niemenmaa wrote:
Setting buildenv twice was done on purpose in commit 02a0bf550a22e199f48537b7eee87361b112e8a0, but at a glance I'm not sure whether it provides any benefit. All the meaningful changes are to environment variables that are exported from the parent process anyway, so all the changes are either repeated or even duplicated (like in PATH, or this CCACHE_PREFIX issue). Maybe it would be better to simply remove the prepare_buildenv call from the INFAKEROOT case in makepkg.sh.in?
makepkg --repackage will skip the build() step, and some package() functions actually result in artifacts being rebuilt (not all build systems handle this properly).
While it's true we run prepare_buildenv() before the fakeroot either way, so exported variables are still exported, makepkg.conf will often (usually?) reset them, and we need to specially handle DEBUG_*FLAGS as well as -fdebug-prefix-map.
The other major problem I see is that we currently have buildenv/buildflags.sh.in and buildenv/makeflags.sh.in which actually unset variables that may be restored during fakeroot due to reparsing makepkg.conf
package() functions, even when --repackage is *not* used, should still respect MAKEFLAGS when running make install, even if you discount needing to have consistent CFLAGS when running a dirty package() function.
Right, so it's not as simple as I hoped. The interaction with makepkg.conf is something I didn't realize. I hope there's still some sort of nicer alternative to these kinds of pinpoint fixes, but I guess it wouldn't happen in the short term anyway.
On 1/7/20 2:51 AM, Matti Niemenmaa wrote:
From: Matti Niemenmaa <matti.niemenmaa+git@iki.fi>
buildenv is set once for build() and a second time for package(). When using both distcc and ccache, this lead to CCACHE_PREFIX="distcc distcc" in package(), which breaks PKGBUILDs that execute the compiler in package() because distcc complains:
distcc[383041] (main) CRITICAL! distcc seems to have invoked itself recursively!
Avoid causing this error by only adding "distcc" to CCACHE_PREFIX if it's not yet there.
Signed-off-by: Matti Niemenmaa <matti.niemenmaa+git@iki.fi> --- scripts/libmakepkg/buildenv/compiler.sh.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ncurses is an example of a package that runs into this problem.
Setting buildenv twice was done on purpose in commit 02a0bf550a22e199f48537b7eee87361b112e8a0, but at a glance I'm not sure whether it provides any benefit. All the meaningful changes are to environment variables that are exported from the parent process anyway, so all the changes are either repeated or even duplicated (like in PATH, or this CCACHE_PREFIX issue). Maybe it would be better to simply remove the prepare_buildenv call from the INFAKEROOT case in makepkg.sh.in?
Apparently there was some confusion if this patch has a problem. To clarify, my previous comment was rejecting this possible alternative, which the patch does not in fact implement.
diff --git a/scripts/libmakepkg/buildenv/compiler.sh.in b/scripts/libmakepkg/buildenv/compiler.sh.in index 69f58a29..c93c77b4 100644 --- a/scripts/libmakepkg/buildenv/compiler.sh.in +++ b/scripts/libmakepkg/buildenv/compiler.sh.in @@ -44,7 +44,9 @@ buildenv_ccache() { buildenv_distcc() { if check_buildoption "distcc" "y"; then if (( using_ccache )); then - export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + if ! [[ "$CCACHE_PREFIX" =~ (^| )distcc($| ) ]]; then
Regex here feels a bit overkill, I'm wondering if maybe it would be a better idea to do: [[ " $CCACHE_PREFIX " = *' distcc '* ]] (Spaces assume that we think there is a valid use case for idk, CCACHE_PREFIX=/usr/bin/notdistcc-foo and therefore want to match a word, specifically.)
+ export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + fi export CCACHE_BASEDIR="$srcdir" elif [[ -d /usr/lib/distcc/bin ]]; then export PATH="/usr/lib/distcc/bin:$PATH"
-- Eli Schwartz Bug Wrangler and Trusted User
On 30/01/2021 00.18, Eli Schwartz wrote:
On 1/7/20 2:51 AM, Matti Niemenmaa wrote:
diff --git a/scripts/libmakepkg/buildenv/compiler.sh.in b/scripts/libmakepkg/buildenv/compiler.sh.in index 69f58a29..c93c77b4 100644 --- a/scripts/libmakepkg/buildenv/compiler.sh.in +++ b/scripts/libmakepkg/buildenv/compiler.sh.in @@ -44,7 +44,9 @@ buildenv_ccache() { buildenv_distcc() { if check_buildoption "distcc" "y"; then if (( using_ccache )); then - export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + if ! [[ "$CCACHE_PREFIX" =~ (^| )distcc($| ) ]]; then
Regex here feels a bit overkill, I'm wondering if maybe it would be a better idea to do:
[[ " $CCACHE_PREFIX " = *' distcc '* ]]
(Spaces assume that we think there is a valid use case for idk, CCACHE_PREFIX=/usr/bin/notdistcc-foo and therefore want to match a word, specifically.)
Yeah, that works equally well. At the moment the regex solution still feels clearer to me but I'm not attached to it — posted a v2.
From: Matti Niemenmaa <matti.niemenmaa+git@iki.fi> buildenv is set once for build() and a second time for package(). When using both distcc and ccache, this lead to CCACHE_PREFIX="distcc distcc" in package(), which breaks PKGBUILDs that execute the compiler in package() because distcc complains: distcc[383041] (main) CRITICAL! distcc seems to have invoked itself recursively! Avoid causing this error by only adding "distcc" to CCACHE_PREFIX if it's not yet there. Signed-off-by: Matti Niemenmaa <matti.niemenmaa+git@iki.fi> --- scripts/libmakepkg/buildenv/compiler.sh.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) This time using globbing instead of regex, as requested by Eli. diff --git a/scripts/libmakepkg/buildenv/compiler.sh.in b/scripts/libmakepkg/buildenv/compiler.sh.in index 69f58a29..b1e36f56 100644 --- a/scripts/libmakepkg/buildenv/compiler.sh.in +++ b/scripts/libmakepkg/buildenv/compiler.sh.in @@ -44,7 +44,9 @@ buildenv_ccache() { buildenv_distcc() { if check_buildoption "distcc" "y"; then if (( using_ccache )); then - export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + if [[ " $CCACHE_PREFIX " != *" distcc "* ]]; then + export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + fi export CCACHE_BASEDIR="$srcdir" elif [[ -d /usr/lib/distcc/bin ]]; then export PATH="/usr/lib/distcc/bin:$PATH" -- 2.30.0
On 31/1/21 3:46 am, Matti Niemenmaa wrote:
From: Matti Niemenmaa <matti.niemenmaa+git@iki.fi>
buildenv is set once for build() and a second time for package(). When using both distcc and ccache, this lead to CCACHE_PREFIX="distcc distcc" in package(), which breaks PKGBUILDs that execute the compiler in package() because distcc complains:
distcc[383041] (main) CRITICAL! distcc seems to have invoked itself recursively!
Avoid causing this error by only adding "distcc" to CCACHE_PREFIX if it's not yet there.
Signed-off-by: Matti Niemenmaa <matti.niemenmaa+git@iki.fi>
Thanks for chasing this up. A
--- scripts/libmakepkg/buildenv/compiler.sh.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
This time using globbing instead of regex, as requested by Eli.
diff --git a/scripts/libmakepkg/buildenv/compiler.sh.in b/scripts/libmakepkg/buildenv/compiler.sh.in index 69f58a29..b1e36f56 100644 --- a/scripts/libmakepkg/buildenv/compiler.sh.in +++ b/scripts/libmakepkg/buildenv/compiler.sh.in @@ -44,7 +44,9 @@ buildenv_ccache() { buildenv_distcc() { if check_buildoption "distcc" "y"; then if (( using_ccache )); then - export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + if [[ " $CCACHE_PREFIX " != *" distcc "* ]]; then + export CCACHE_PREFIX="${CCACHE_PREFIX:+$CCACHE_PREFIX }distcc" + fi export CCACHE_BASEDIR="$srcdir" elif [[ -d /usr/lib/distcc/bin ]]; then export PATH="/usr/lib/distcc/bin:$PATH"
participants (3)
-
Allan McRae
-
Eli Schwartz
-
Matti Niemenmaa