Closed
Bug 416529
Opened 16 years ago
Closed 16 years ago
Don't include .deps/.all.pp when not needed
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
People
(Reporter: jag+mozilla, Assigned: jag+mozilla)
Details
Attachments
(1 file, 1 obsolete file)
1.85 KB,
patch
|
benjamin
:
superreview+
pavlov
:
approval1.9+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•16 years ago
|
||
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.
Assignee | ||
Comment 2•16 years ago
|
||
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 3•16 years ago
|
||
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+
Assignee | ||
Comment 4•16 years ago
|
||
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)
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #302407 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 8•16 years ago
|
||
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.
Description
•