480069 - Recursive submakes without shell loops

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Benjamin Smedberg]

Attachment: Recurse using a single command for each directory, rev. 1

If recursive submake commands use shell features like loops or set -e, pymake can't recurse within a single process. This patch switches to using a separate commands for each dir in DIRS, TOOL_DIRS, etc, so that pymake can do all of mozilla (except for NSPR and NSS) within a single pymake process.

This doesn't hurt gmake at all (it may help, in fact). Passes tryserver.

Comment 1

[(not currently active) Ted Mielczarek]

Comment on attachment 364014 [details] [diff] [review]
Recurse using a single command for each directory, rev. 1

Heh, this patch really confused bugzilla's diff view. I was looking at the define SUBMAKE bit, and it looked like it was defined to nothing. :)

Also, related, I think we can just remove MAKE = @MAKE@ from autoconf.mk.in, no? It's a special variable anyway. I'm not sure why it's even there, historical reasons? It was added in bug 201150, but AFAICT, GNU Make has always supported $(MAKE).

+define SUBMAKE
++@$(MAKE) $(if $(1),-C $(1)) $(2)

The optional parameter 1 makes the non-dir call of this a little weird to read:
$(foreach tier,$(TIERS),$(call SUBMAKE,,tier_$(tier)))

Maybe swap the order of the params to make that read better? I'm not really sure.

Also, you dropped the $(UPDATE_TITLE) here. I know some people like that feature, so you should keep it in SUBMAKE. (It's off by default anyway, with a configure option.)

-ifdef ALL_PLATFORMS
-all_platforms:: $(NFSPWD)

Good call. I was thinking about tearing all this crap out when I hit that nfspwd build bustage in the first place.

+%:: %.c $(GLOBAL_DEPS)

What's the impetus behind these changes?

 zipmakes:
 ifneq (,$(filter $(PROGRAM) $(SIMPLE_PROGRAMS) $(LIBRARY) $(SHARED_LIBRARY),$(TARGETS)))
 	zip $(DEPTH)/makefiles $(subst $(topsrcdir),$(MOZ_SRC)/mozilla,$(srcdir)/Makefile.in)
 endif
-	+$(LOOP_OVER_PARALLEL_DIRS)
-	+$(LOOP_OVER_DIRS)
+	$(LOOP_OVER_PARALLEL_DIRS)
+	$(LOOP_OVER_DIRS)

Just remove the zipmakes target while you're there? I suspect nobody has used it in ages anyway.

Comment 2

[(not currently active) Ted Mielczarek]

I don't think there's any reason we can't take the same approach in NSPR's build system as well. The LOOP_OVER_DIRS there is unnecessarily complicated. (It checks test -d for every entry in DIRS, currently!) You could do that in a followup bug though, if you'd like.

Comment 3

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/3291e357eb4a

Comment 4

[Benjamin Smedberg]

Oh, to answer the questions in-bug instead of just IRC:

Making the match-anything rules double-colon means that they are "terminal": they are never chained with other implicit rules. This reduces the number of targets that must be considered significantly.

Comment 5

[Serge Gautherie (:sgautherie)]

What about 1.9.1?

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> What about 1.9.1?

I guess 'wanted1.9.1-' answers that, at last.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> I don't think there's any reason we can't take the same approach in NSPR's
> build system as well. The LOOP_OVER_DIRS there is unnecessarily complicated.
> (It checks test -d for every entry in DIRS, currently!) You could do that in a
> followup bug though, if you'd like.

Was that bug filed?

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to comment #7)
> Was that bug filed?

It wound up being bug 525221.

Comment 9

[[e-mail]]

(In reply to comment #3)
> http://hg.mozilla.org/mozilla-central/rev/3291e357eb4a
Thanks for fixing xterm updates (not shown in attachment 364014 [details] [diff] [review]).