Missing EXIT_ON_ERROR in Makefiles

NEW
Unassigned

Status

P3
normal
13 years ago
4 years ago

People

(Reporter: iwj, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
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:

Comment 1

13 years ago
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

Comment 2

13 years ago
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
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.