Last Comment Bug 703930 - Add |set -o errexit| to *makefiles.sh to abort the build on errors rather than just hiding them
: Add |set -o errexit| to *makefiles.sh to abort the build on errors rather tha...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Ed Morley [:emorley]
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-19 15:46 PST by Ed Morley [:emorley]
Modified: 2011-12-01 04:43 PST (History)
5 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.02 KB, patch)
2011-11-19 17:03 PST, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2011-11-19 15:46:50 PST
As suggested by Joey, to avoid typos such as that fixed by bug 698526 slipping through again, we should ensure the *makefiles.sh scripts bail with a non-zero exit code and thus abort the build, if something goes awry.

http://www.davidpashley.com/articles/writing-robust-shell-scripts.html says we can do this with |set -e| or |set -o errexit|. I prefer the latter since it's more readable.
Comment 1 Ed Morley [:emorley] 2011-11-19 17:03:13 PST
Created attachment 575716 [details] [diff] [review]
Patch v1

|set -o errexit| is set for all scripts from that point onwards, so has to be turned off at the end of allmakefiles.sh, otherwise multiple places in the rest of configure will error out (eg: the |mv -f config/autoconf.mk config/autoconf.mk.orig 2> /dev/null| line shortly after allmakefile.sh is executed, and many more).
Comment 2 Ed Morley [:emorley] 2011-11-19 17:06:11 PST
Meant to say, deliberately making a typo in services/makefiles.sh with this patch applied, correctly gives:
{
../inbound/services/makefiles.sh: line 56: add_FOOmakefiles: command not found
*** Fix above errors and then restart with               "c:/mozilla-build/python/python.exe c:/mozilla/repos/inbound/build/pymake/pymake/../make.py -f client.mk build"
c:\mozilla\repos\inbound\client.mk:315:0: command 'cd . &&   ../inbound/configure  \
  || ( echo "*** Fix above errors and then restart with\
               \"c:/mozilla-build/python/python.exe c:/mozilla/repos/inbound/build/pymake/pymake/../make.py -f client.mk build\"" && exit 1 )' failed, return code 1
c:\mozilla\repos\inbound\client.mk:327:0: command 'c:/mozilla-build/python/python.exe c:/mozilla/repos/inbound/build/pymake/pymake/../make.py -f ../inbound/client.mk configure' failed, return code 2
c:\mozilla\repos\inbound\client.mk:174:0: command 'c:/mozilla-build/python/python.exe c:/mozilla/repos/inbound/build/pymake/pymake/../make.py -f ../inbound/client.mk realbuild' failed, return code 2
}
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-21 04:56:26 PST
Comment on attachment 575716 [details] [diff] [review]
Patch v1

Review of attachment 575716 [details] [diff] [review]:
-----------------------------------------------------------------

Is this going to make the "people remove a directory from tree but leave it in *-makefiles.sh" case error out?
Comment 4 Ed Morley [:emorley] 2011-11-21 05:08:01 PST
It shouldn't, since errexit will be turned off at the end of allmakefiles.sh, before configure calls http://mxr.mozilla.org/mozilla-central/source/build/autoconf/acoutput-fast.pl#174

Do you want it to?
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-21 05:08:50 PST
I don't really care either way, I was just curious.
Comment 7 Marco Bonardo [::mak] 2011-12-01 04:43:27 PST
https://hg.mozilla.org/mozilla-central/rev/0f4282490c81

Note You need to log in before you can comment on or make changes to this bug.