| Message ID | 20200107075100.422553-1-matti.niemenmaa+pacman-dev@iki.fi |
|---|---|
| State | New |
| Headers | show |
| Series | [pacman-dev] makepkg: Don't double-layer distcc on ccache | expand |
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" >
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" >
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.
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"