[pacman-dev] [PATCH] Ensure build failure if a single build() command fails
Hi, I doubt this was by intention: errors in build() functions are only fatal, if "--log" is enabled. I just made a buggy pkg because some "install ..." commands where not handled by "|| return 1". Our PKGBUILDs are cluttert full of "|| return 1". Failing commands in build functions should always result in an build error. Signed-off-by: Juergen Hoetzel <juergen@archlinux.org> --- scripts/makepkg.sh.in | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f46b7f8..84d4599 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -703,6 +703,7 @@ run_build() { local ret=0 if [ "$LOGGING" -eq 1 ]; then BUILDLOG="${startdir}/${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log" + BUILDLOG_CMD="tee $BUILDLOG" if [ -f "$BUILDLOG" ]; then local i=1 while true; do @@ -714,11 +715,11 @@ run_build() { done mv "$BUILDLOG" "$BUILDLOG.$i" fi - - build 2>&1 | tee "$BUILDLOG"; ret=${PIPESTATUS[0]} else - build 2>&1 || ret=$? + BUILDLOG_CMD="cat -" fi + + build 2>&1 | ${BUILDLOG_CMD}; ret=${PIPESTATUS[0]} # reset our shell options eval "$shellopts" -- 1.6.3.2
On Fri, Jun 12, 2009 at 4:49 PM, Jürgen Hötzel<juergen@hoetzel.info> wrote:
Hi,
I doubt this was by intention:
errors in build() functions are only fatal, if "--log" is enabled.
I just made a buggy pkg because some "install ..." commands where not handled by "|| return 1".
Our PKGBUILDs are cluttert full of "|| return 1". Failing commands in build functions should always result in an build error.
Might be worthwhile to add a source comment indicating something about this. I don't know the details, but it looks to be the difference between $PIPESTATUS[0] and $?
On Fri, Jun 12, 2009 at 04:55:35PM -0500, Aaron Griffin wrote:
Might be worthwhile to add a source comment indicating something about this. I don't know the details, but it looks to be the difference between $PIPESTATUS[0] and $?
Yep: build 2>&1 | tee "$BUILDLOG"; ret=${PIPESTATUS[0]} ^^^^^ forked off, ERR trap is handled in new process, thus returning 1 build 2>&1 || ret=$? ^^^^^^ failing commands part of `||' never trigger ERR traps (would end "makepkg" in this case) Jürgen _____________________________
pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
Jürgen Hötzel wrote:
Hi,
I doubt this was by intention:
errors in build() functions are only fatal, if "--log" is enabled.
I just made a buggy pkg because some "install ..." commands where not handled by "|| return 1".
Our PKGBUILDs are cluttert full of "|| return 1". Failing commands in build functions should always result in an build error.
Signed-off-by: Juergen Hoetzel <juergen@archlinux.org> --- scripts/makepkg.sh.in | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f46b7f8..84d4599 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -703,6 +703,7 @@ run_build() { local ret=0 if [ "$LOGGING" -eq 1 ]; then BUILDLOG="${startdir}/${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log" + BUILDLOG_CMD="tee $BUILDLOG" if [ -f "$BUILDLOG" ]; then local i=1 while true; do @@ -714,11 +715,11 @@ run_build() { done mv "$BUILDLOG" "$BUILDLOG.$i" fi - - build 2>&1 | tee "$BUILDLOG"; ret=${PIPESTATUS[0]} else - build 2>&1 || ret=$? + BUILDLOG_CMD="cat -" fi + + build 2>&1 | ${BUILDLOG_CMD}; ret=${PIPESTATUS[0]} # reset our shell options eval "$shellopts"
Seems fine. We will want to do the "cat -" thing in run_package too to catch packaging errors. In fact, as the tee mechanism is so different there, we will need to check that it actually catches errors... Allan
On Fri, Jun 12, 2009 at 9:02 PM, Allan McRae<allan@archlinux.org> wrote:
Jürgen Hötzel wrote:
Hi,
I doubt this was by intention:
errors in build() functions are only fatal, if "--log" is enabled. I just made a buggy pkg because some "install ..." commands where not handled by "|| return 1".
Our PKGBUILDs are cluttert full of "|| return 1". Failing commands in build functions should always result in an build error. Signed-off-by: Juergen Hoetzel <juergen@archlinux.org> --- scripts/makepkg.sh.in | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f46b7f8..84d4599 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -703,6 +703,7 @@ run_build() { local ret=0 if [ "$LOGGING" -eq 1 ]; then
BUILDLOG="${startdir}/${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log" + BUILDLOG_CMD="tee $BUILDLOG" if [ -f "$BUILDLOG" ]; then local i=1 while true; do @@ -714,11 +715,11 @@ run_build() { done mv "$BUILDLOG" "$BUILDLOG.$i" fi - - build 2>&1 | tee "$BUILDLOG"; ret=${PIPESTATUS[0]} else - build 2>&1 || ret=$? + BUILDLOG_CMD="cat -" fi + + build 2>&1 | ${BUILDLOG_CMD}; ret=${PIPESTATUS[0]} # reset our shell options eval "$shellopts"
Seems fine. We will want to do the "cat -" thing in run_package too to catch packaging errors. In fact, as the tee mechanism is so different there, we will need to check that it actually catches errors...
Should I wait to apply this then, or take it for now and wait for another patch? -Dan
Dan McGee wrote:
On Fri, Jun 12, 2009 at 9:02 PM, Allan McRae<allan@archlinux.org> wrote:
Jürgen Hötzel wrote:
Hi,
I doubt this was by intention:
errors in build() functions are only fatal, if "--log" is enabled. I just made a buggy pkg because some "install ..." commands where not handled by "|| return 1".
Our PKGBUILDs are cluttert full of "|| return 1". Failing commands in build functions should always result in an build error. Signed-off-by: Juergen Hoetzel <juergen@archlinux.org> --- scripts/makepkg.sh.in | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f46b7f8..84d4599 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -703,6 +703,7 @@ run_build() { local ret=0 if [ "$LOGGING" -eq 1 ]; then
BUILDLOG="${startdir}/${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log" + BUILDLOG_CMD="tee $BUILDLOG" if [ -f "$BUILDLOG" ]; then local i=1 while true; do @@ -714,11 +715,11 @@ run_build() { done mv "$BUILDLOG" "$BUILDLOG.$i" fi - - build 2>&1 | tee "$BUILDLOG"; ret=${PIPESTATUS[0]} else - build 2>&1 || ret=$? + BUILDLOG_CMD="cat -" fi + + build 2>&1 | ${BUILDLOG_CMD}; ret=${PIPESTATUS[0]} # reset our shell options eval "$shellopts"
Seems fine. We will want to do the "cat -" thing in run_package too to catch packaging errors. In fact, as the tee mechanism is so different there, we will need to check that it actually catches errors...
Should I wait to apply this then, or take it for now and wait for another patch?
I like to keep the run_build and run_package functions as similar as possible (one day I might get around to refactoring them... especially as I want to add run_check it the future). So I would prefer to wait until this is fix for both functions in as similar way as possible. Allan
On Sat, Jun 13, 2009 at 10:01 AM, Allan McRae<allan@archlinux.org> wrote:
Dan McGee wrote:
On Fri, Jun 12, 2009 at 9:02 PM, Allan McRae<allan@archlinux.org> wrote:
Jürgen Hötzel wrote:
Hi,
I doubt this was by intention:
errors in build() functions are only fatal, if "--log" is enabled. I just made a buggy pkg because some "install ..." commands where not handled by "|| return 1".
Our PKGBUILDs are cluttert full of "|| return 1". Failing commands in build functions should always result in an build error. Signed-off-by: Juergen Hoetzel <juergen@archlinux.org> --- scripts/makepkg.sh.in | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f46b7f8..84d4599 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -703,6 +703,7 @@ run_build() { local ret=0 if [ "$LOGGING" -eq 1 ]; then
BUILDLOG="${startdir}/${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log" + BUILDLOG_CMD="tee $BUILDLOG" if [ -f "$BUILDLOG" ]; then local i=1 while true; do @@ -714,11 +715,11 @@ run_build() { done mv "$BUILDLOG" "$BUILDLOG.$i" fi - - build 2>&1 | tee "$BUILDLOG"; ret=${PIPESTATUS[0]} else - build 2>&1 || ret=$? + BUILDLOG_CMD="cat -" fi + + build 2>&1 | ${BUILDLOG_CMD}; ret=${PIPESTATUS[0]} # reset our shell options eval "$shellopts"
Seems fine. We will want to do the "cat -" thing in run_package too to catch packaging errors. In fact, as the tee mechanism is so different there, we will need to check that it actually catches errors...
Should I wait to apply this then, or take it for now and wait for another patch?
I like to keep the run_build and run_package functions as similar as possible (one day I might get around to refactoring them... especially as I want to add run_check it the future). So I would prefer to wait until this is fix for both functions in as similar way as possible.
Address this before 3.3? We never got a resubmit. -Dan
Allan McRae wrote:
Jürgen Hötzel wrote:
Hi,
I doubt this was by intention:
errors in build() functions are only fatal, if "--log" is enabled. I just made a buggy pkg because some "install ..." commands where not handled by "|| return 1".
Our PKGBUILDs are cluttert full of "|| return 1". Failing commands in build functions should always result in an build error.
Signed-off-by: Juergen Hoetzel <juergen@archlinux.org> --- scripts/makepkg.sh.in | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f46b7f8..84d4599 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -703,6 +703,7 @@ run_build() { local ret=0 if [ "$LOGGING" -eq 1 ]; then
BUILDLOG="${startdir}/${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log" + BUILDLOG_CMD="tee $BUILDLOG" if [ -f "$BUILDLOG" ]; then local i=1 while true; do @@ -714,11 +715,11 @@ run_build() { done mv "$BUILDLOG" "$BUILDLOG.$i" fi - - build 2>&1 | tee "$BUILDLOG"; ret=${PIPESTATUS[0]} else - build 2>&1 || ret=$? + BUILDLOG_CMD="cat -" fi + + build 2>&1 | ${BUILDLOG_CMD}; ret=${PIPESTATUS[0]} # reset our shell options eval "$shellopts"
Seems fine. We will want to do the "cat -" thing in run_package too to catch packaging errors. In fact, as the tee mechanism is so different there, we will need to check that it actually catches errors...
I was right about needing to check the run_package function. It does not catch errors even when run with logging. I am working on fixing up this patch. Allan
participants (4)
-
Aaron Griffin
-
Allan McRae
-
Dan McGee
-
Jürgen Hötzel