Closed Bug 416529 Opened 16 years ago Closed 16 years ago

Don't include .deps/.all.pp when not needed

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jag+mozilla, Assigned: jag+mozilla)

Details

Attachments

(1 file, 1 obsolete file)

Currently we unconditionally include .deps/.all.pp which causes its target to be triggered, regenerating the file for each pass through the build (export, libs, tools). We really only need to generate and include it for the libs phase. bsmedberg suggested using the MAKECMDGOALS variable (contains the targets passed to make) for this.

The filter-out list specifies for which targets we don't need to include the dependencies:
 - |export|, |*clean| and |clobber*| are easy to understand
 - |default| and |all| walk the tree with |export|, |libs|, and |tools|
 - |tools| calls make |libs| for each of the tool dirs

Neither |default|, |all| nor |tools| need the dependencies themselves, and because they end up calling make with |libs| to do the actual building, MAKECMDGOALS will be "libs" at that point and trigger including the .pp file(s).
Attachment #302281 - Flags: superreview?(benjamin)
Status: NEW → ASSIGNED
Comment on attachment 302281 [details] [diff] [review]
Don't include .deps/.all.pp if we're doing export, tools, or one of the cleans.

I'll be adding |chrome| and |realchrome| to that list.
So it turns out that if .all.pp gets updated, we try to update it again, because make starts from the top if you update any included files (like we're doing here). The second time around, while we do run mddepend.pl, we don't actually update .all.pp (content is the same) so at least there's no endless cycle, but it'd be nice to avoid doing all the legwork only to throw it away. $(MAKE_RESTARTS), available in 3.81 and up, gives us that. It's empty for the first round, a counter for every triggered restart.
Attachment #302281 - Attachment is obsolete: true
Attachment #302407 - Flags: superreview?(benjamin)
Attachment #302281 - Flags: superreview?(benjamin)
Comment on attachment 302407 [details] [diff] [review]
Add chrome and realchrome to previous patch, and avoid generating .all.pp when make re-executes due to updated include

I'm a little worried about the MAKE_RESTARTS thing... there are multiple reasons we could need to restart make, including a changed Makefile.in: in that case, could we end up skipping rebuilding .all.pp?
Attachment #302407 - Flags: superreview?(benjamin) → superreview+
make will include everything first, triggering the .all.pp target in our case, and then it checks to see if any of the included files was changed, and if so it'll restart at that point. Makefile is handled the same way, so there's only one restart. So that's mostly a good thing, except if Makefile.in changed in a way that affects the building of .all.pp, then we won't pick that up.

How about something like:

ifeq (,$(MAKE_RESTART))
$(MDDEPDIR)/.all.pp: FORCE
endif

$(MDDEPDIR)/.all.pp: Makefile Makefile.in $(BUILD_TOOLS)/mddepend.pl
        @$(PERL) $(BUILD_TOOLS)/mddepend.pl $@ $(MDDEPEND_FILES)
Hrm, that might not work perfectly. If Makefile's modtime ends up the same as .all.pp's then we won't update .all.pp according to the new Makefile.

That could probably be fixed by adding $(RM) $(MDDEPDIR)/.all.pp to the Makefile target in rules.mk, in which case we don't need the Makefile and Makefile.in dependencies, but we still need the separately guarded FORCE as in comment 4.
Not sure we really need to have .all.pp depend on Makefile though. It's very unlikely we'll change the values of PERL or BUILD_TOOLS, or change the meaning of MDDEPEND_FILES.
Comment on attachment 302407 [details] [diff] [review]
Add chrome and realchrome to previous patch, and avoid generating .all.pp when make re-executes due to updated include

We discussed the previous comments on IRC.

To clarify, |make| will process all included files, then check all of them (with Makefile last) to see if they match any targets, executing their commands if necessary. If, as a result of all of this, any of the included files has been modified, make does a restart. See http://www.gnu.org/software/make/manual/make.html#Remaking-Makefiles

So in our case, if Makefile.in was modified, make updates .all.pp first ( : FORCE, MAKE_RESTARTS is empty), then Makefile ( : Makefile.in), and then it restarts.

We decided not to worry about having .all.pp updated when running the updated Makefile, see also comment 6.
Attachment #302407 - Flags: approval1.9?
Attachment #302407 - Flags: approval1.9? → approval1.9+
Checking in config/rules.mk;
/cvsroot/mozilla/config/rules.mk,v  <--  rules.mk
new revision: 3.588; previous revision: 3.587
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: