User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20051007 Debian/1.7.12-1 Build Identifier: firefox 1.4.99+1.5rc3.dfsg-1ubuntu4 If one of the commands invoked during the build system's `for' loops fails, the build can carry on, oblivious to the failure. For example, security/nss/lib/Makefile contains this code: export:: if test -d $(CORE_DEPTH)/../../ns/security/lib/crypto; then \ $(NSINSTALL) -D crypto; \ for file in $(FILES) ; do \ etc. If this fails for any reason, the loop will continue and make can remain oblivious to the failure, and will not stop the build. Every multi-line shell command in every makefile (including .mk files) should start with `set -e', which appears to be available as @$(EXIT_ON_ERROR). Reproducible: Always Steps to Reproduce:
As filed this bug is about the NSS build system, not the mozilla build system. If you see issues like this in the moz build system also, please file a separate bug (and please provide references, I certainly don't want to go hand-picking through makefiles on a multi-command-shell-hunt).
Assignee: nobody → wtchang
Status: UNCONFIRMED → NEW
Component: Build Config → Build
Ever confirmed: true
Product: Firefox → NSS
QA Contact: build.config → wtchang
Created attachment 205983 [details] [diff] [review] Proposed patch Thanks for the bug report. The specific makefile snippet you pointed out is dead code, so the review of this patch will have a low priority. This patch fixes the bug by removing the dead code. Unfortunately I have to remove the other related dead code. 1. security/coreconf/rules.mk: I make $(SHARED_LIBRARY) depend on $(EXTRA_LIBS), just like $(PROGRAM) does. This replaces the dependency of $(SHARED_LIBRARY) on $(CRYPTOLIB) in security/nss/lib/softoken/config.mk and security/nss/lib/ssl/config.mk. This is the right thing to do, unfortunately we need to review all instances of $(EXTRA_LIBS) to make sure their values are suitable as dependencies. (For example, they can't be defined as "-lfoo -lbar".) 2. security/nss/cmd/platlibs.mk: remove obsolete MOZILLA_SECURITY_BUILD and MOZILLA_BSAFE_BUILD code. Substitute $(CRYPTOLIB) in place. 3. security/nss/lib/Makefile: remove obsolete fortcrypt and MOZILLA_SECURITY_BUILD code. 4. security/nss/lib/nss/config.mk: update comments, which must have been copied from security/nss/cmd/platlibs.mk. We build $(SHARED_LIBRARY), not $(PROGRAM), in this directory. Note that the new comments are true only if the changes to #1 are made. 5. security/nss/lib/softoken/config.mk and security/nss/lib/ssl/config.mk: remove obsolete MOZILLA_SECURITY_BUILD code. Substitute $(CRYPTOLIB) in place. Update comments (see #4).
Attachment #205983 - Flags: review?(nelson)
Comment on attachment 205983 [details] [diff] [review] Proposed patch Comments on Proposed patch: I agree with all the changes in this patch, except for one comment, which looked wrong and led me to a discovery. In the patch to security/nss/lib/nss/config.mk we see: >-# $(PROGRAM) has explicit dependencies on $(EXTRA_LIBS) >+# $(SHARED_LIBRARY) has explicit dependencies on $(EXTRA_LIBS) > SHARED_LIBRARY_LIBS = \ > $(DIST)/lib/$(LIB_PREFIX)certhi.$(LIB_SUFFIX) \ Neither the old comment nor the new one are relevant to the symbol defined immediately below them. So, I went looking for the uses of SHARED_LIBRARY_LIBS and found that it is used in only one place, in an ifdef! It's value doesn't matter, only its existence. So, defining it as a list of library names (as done above) is pointless. Indeed, I found one makefile that defines that symbol to the string "yes". The makefile that uses SHARED_LIBRARY_LIBS is in coreconf/rules.mk. > 307 ifdef SHARED_LIBRARY_LIBS > 308 ifdef BUILD_TREE > 309 SUB_SHLOBJS = $(foreach dir,$(SHARED_LIBRARY_DIRS),$(shell $(MAKE) -C $(dir) --no-print-directory get_objs)) > 310 else > 311 SUB_SHLOBJS = $(foreach dir,$(SHARED_LIBRARY_DIRS),$(addprefix $(dir)/,$(shell $(MAKE) -C $(dir) --no-print-directory get_objs))) > 312 endif > 313 endif Seems to me that SHARED_LIBRARY_LIBS can be eliminated completely, both here and in nss/config.mk (and anywhere else it is now defined) and line 307 above should be changed to > 307 ifdef SHARED_LIBRARY_DIRS Wan-Teh, Do you agree with that conclusion? If so, then let's eliminate SHARED_LIBRARY_LIBS, too. If not, then r+=nelson for this patch.
Comment on attachment 205983 [details] [diff] [review] Proposed patch I think this patch is fine as far as it goes, but I think we can clean up much more than this. Please see and reply to my comment 3.
Attachment #205983 - Flags: review?(nelson) → review+
Assignee: wtchang → nobody
QA Contact: wtchang → build
You need to log in before you can comment on or make changes to this bug.