Closed Bug 480069 Opened 11 years ago Closed 11 years ago

Recursive submakes without shell loops

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: benjamin, Assigned: benjamin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attachment #364014 - Flags: review?(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.
Attachment #364014 - Flags: review?(ted.mielczarek) → review+
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.
http://hg.mozilla.org/mozilla-central/rev/3291e357eb4a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
What about 1.9.1?
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1-
(In reply to comment #5)
> What about 1.9.1?

I guess 'wanted1.9.1-' answers that, at last.
(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?
Flags: in-testsuite-
(In reply to comment #7)
> Was that bug filed?

It wound up being bug 525221.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.