Closed
Bug 462463
Opened 16 years ago
Closed 12 years ago
make dependencies smarter, get rid of mddepend.pl
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla23
People
(Reporter: ted, Assigned: glandium)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [buildfaster:p3])
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
The provided URL shows a way to create smart dependencies for GNU make such that object files will be rebuilt if source or header files that they depend on are deleted. The trick is to make the source and header files targets in rules with no prerequisites and no commands. This tells make that if the file doesn't exist, it needs to rebuild anything that depends on it. We currently handle this by running mddepend.pl every single time we enter a directory, and having it stat all the files in the dependency files. This sucks. We could instead just post-process the dependency files and add the necessary rules right after the file is compiled, and ditch mddepend.pl completely. For clarity, the rules will look like this: Normal depends file: foo.o: foo.c foo.h We will add these rules: foo.c: foo.h:
Comment 1•16 years ago
|
||
I've got plane-time to look at this: should be simple!
Assignee: nobody → benjamin
Reporter | ||
Comment 2•16 years ago
|
||
Some drive-by comments from the changes here: http://hg.mozilla.org/users/bsmedberg_mozilla.com/mddepend-removal/rev/79b8b8bc7de1 3.18 +MDDEPFILES += $(call GETDEPFILES,$(OBJS)) 3.19 +MDDEPFILES += $(call GETDEPFILES,$(XPIDLSRCS) $(SDK_XPIDLSRCS)) 3.20 +MDDEPFILES += $(call GETDEPFILES,$(SIMPLE_PROGRAMS)) You could just collapse this into one assignment with continuations. 3.47 +MDDEPFILE = $(call GETDEPFILES,$@) 3.48 +MDDEPDIR = $(dir $(_MDDEPFILE)) Is that second MDDEPFILE supposed to start with an underscore? 3.92 - $(MKDEPEND) -o'.$(OBJ_SUFFIX)' -f- $(DEFINES) $(ACDEFINES) $(INCLUDES) $< 2>/dev/null | sed -e "s|^[^ ]*/||" > $(_MDDEPFILE) ; \ 3.93 + $(MKDEPEND) -o'.$(OBJ_SUFFIX)' -f- $(DEFINES) $(ACDEFINES) $(INCLUDES) $< 2>/dev/null | sed -e "s|^[^ ]*/||" > $(MDDEPFILE) ; \ Shouldn't you be piping this through process_depends.py? 3.154 -endif #STRICT_CPLUSPLUS_SUFFIX 3.155 +endif #STRICT_CPLUSPLUS_SUFFI Looks like an accidental removal.
Comment 3•16 years ago
|
||
This should be ready for prime-time
Attachment #346325 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 4•16 years ago
|
||
Comment on attachment 346325 [details] [diff] [review] mddepend removal, rev. 1 +++ b/build/unix/process_depends.py Needs a tri-license header. Also, could you wrap the top-level parts of this in a method, and then use the "if __name__ == '__main__': main()" sort of trick? Then perhaps in our eventual glorious future we can use this as a module. TARGETS += elf-dynstr-gc Any idea why elf-dynstr-gc isn't just in SIMPLE_PROGRAMS? In fact, SIMPLE_PROGRAMS is empty in this file, despite being referenced in TARGETS. Not that you need to fix that, but wondering why all this crud is here. # MDDEPDIR is the subdirectory where all the dependency files are placed. Fix the comment to just say ".deps". Man, we really need to figure out a better solution in js/src/Makefile.in, these per-platform ifdefs to work around optimizer bugs are awful.
Comment 5•16 years ago
|
||
ok, new cset ready for review: http://hg.mozilla.org/users/bsmedberg_mozilla.com/mddepend-removal/rev/76fed3a0cebd CHANGES: * use order-only dependencies on the .deps directory; the directory mtime changes any time any file is compiled, so you end up causing many more things to rebuild than actually should * fix dependency of .deps creation on the initial compile of nsinstall * use target-specific rules for a lot of the crap in js/src/Makefile.in to avoid replicating rules * add MPL to process_depends.py * allow using process_depends.py as a module I didn't change elf-dynstr-gc because it must be built during the export phase, not the libs phase, and there was a $(PROGRAM) already, and I think I can't do all that with SIMPLE_PROGRAMS.
Reporter | ||
Comment 6•16 years ago
|
||
Looks good, just two nits: 1) You appear to be inconsistent in your usage of '.deps' vs. $(MDDEPDIR). Can you just pick one and apply it consistently? 2) You missed the license header in the js/src/ version of process_depends.py. Is that file in the list of things to be checked for synchronicity in the js/src check rule? Thanks for the cleanup in js/src/Makefile.in. We might have to start requiring build peer approval for changes there to keep it in line. :-P r=me
Reporter | ||
Comment 7•16 years ago
|
||
So, I did a bunch of no-op builds comparing this patch to a stock repo, and it seems like this patch makes the build slower(!) normal no-op build: real 4m52.547s real 4m44.859s real 4m18.453s real 4m18.985s mddepend no-op build: real 5m12.906s real 5m16.937s I ran the last two normal builds after running the mddepend builds, because I ran out of disk space and had to delete some stuff to complete the mddepend build originally, so I thought maybe I had fragmented my HDD or something. I have no idea wtf the problem is here.
Reporter | ||
Comment 8•16 years ago
|
||
Comment on attachment 346325 [details] [diff] [review] mddepend removal, rev. 1 Forgot to mark r+, oops. I would like you to hold off landing this until we can figure out the build slowdown I was seeing though.
Attachment #346325 -
Flags: review?(ted.mielczarek) → review+
Comment 9•16 years ago
|
||
Yeah, wow, it's a lot slower on mac (-j0): unpatched: real 3m40.220s user 1m8.835s sys 1m1.715s patched: real 5m38.297s user 2m12.546s sys 1m19.922s But I saw some C++ be recompiled, so I think the deps may be wrong somehow...
Reporter | ||
Comment 10•14 years ago
|
||
Apparently CPP has a -MP option which will do this for you: http://gcc.gnu.org/onlinedocs/gcc-4.5.0/gcc/Preprocessor-Options.html#Preprocessor-Options "-MP This option instructs CPP to add a phony target for each dependency other than the main file, causing each to depend on nothing. These dummy rules work around errors make gives if you remove header files without updating the Makefile to match. "
I might pick this up eventually when I have time, but I think it's safe to assume that bsmedberg isn't actively working on this ATM.
Assignee: benjamin → nobody
Tentatively taking, the WIP I have in my queue for 623617 does this.
Assignee: nobody → khuey
Assignee: khuey → nobody
Updated•12 years ago
|
Whiteboard: [buildfaster:?]
Assignee | ||
Comment 13•12 years ago
|
||
This is WIP. It works as expected, and I ensured all .pp files are regenerated after applying the patch. Only the asm_enc_offsets.s.pp file under media/libvpx is not refreshed, and it shouldn't be a huge problem. Only if one of the file declared as one of its dependencies is removed will there be a problem, but chances are the .pp files would have been refreshed for other reasons before that. One thing that gets broken from this patch is building with sun studio, because it won't get the empty dependencies -MP and the various .py changes bring. I'll add a small script to handle this case. Note mddepend.pl can't be removed from build/unix until c-c switches to not using it.
Updated•12 years ago
|
Whiteboard: [buildfaster:?] → [buildfaster:p3]
Assignee | ||
Comment 14•12 years ago
|
||
Ted for the general thing, gps for the python script that uses pymake's API.
Attachment #724468 -
Flags: review?(ted)
Attachment #724468 -
Flags: review?(gps)
Assignee | ||
Updated•12 years ago
|
Attachment #655332 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
with config/rules.mk in sync.
Attachment #724471 -
Flags: review?(ted)
Attachment #724471 -
Flags: review?(gps)
Assignee | ||
Updated•12 years ago
|
Attachment #724468 -
Attachment is obsolete: true
Attachment #724468 -
Flags: review?(ted)
Attachment #724468 -
Flags: review?(gps)
Assignee | ||
Updated•12 years ago
|
Attachment #346325 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Attachment #724469 -
Flags: review?(ted) → review+
Reporter | ||
Comment 18•12 years ago
|
||
Comment on attachment 724471 [details] [diff] [review] Stop using mddepend.pl Review of attachment 724471 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/unix/add_phony_targets.py @@ +1,1 @@ > +import pymake.data This wants a license header. @@ +5,5 @@ > + > +''' > +Modifies the output of Sun Studio's -xM to look more like the output > +of gcc's -MD -MP, adding phony targets for dependencies. > +''' Feels like a lot of work just to support Sun Studio...
Attachment #724471 -
Flags: review?(ted) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #724473 -
Flags: review?(mbanner)
Comment 19•12 years ago
|
||
Comment on attachment 724473 [details] [diff] [review] Stop using mddepend.pl (c-c) Review of attachment 724473 [details] [diff] [review]: ----------------------------------------------------------------- Not tested, but looks fine to me.
Attachment #724473 -
Flags: review?(mbanner)
Attachment #724473 -
Flags: review?(bugspam.Callek)
Attachment #724473 -
Flags: review+
Comment 20•12 years ago
|
||
Comment on attachment 724471 [details] [diff] [review] Stop using mddepend.pl Review of attachment 724471 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/unix/add_phony_targets.py @@ +13,5 @@ > + print path > + deps = set() > + targets = set() > + for stmt in pymake.parser.parsefile(path): > + if isinstance(stmt, pymake.parserdata.Rule): This won't find rules inside condition blocks. I believe there is an iterstatements() or some such function in parserdata that expands condition blocks so you can find all rules.
Attachment #724471 -
Flags: review?(gps) → review+
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #20) > This won't find rules inside condition blocks. I believe there is an > iterstatements() or some such function in parserdata that expands condition > blocks so you can find all rules. I doubt sun studio is ever going to put condition blocks in the files it generates with -xM :)
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc11223a7745 Will land the c-c part and the removal part when merged to m-c.
Whiteboard: [buildfaster:p3] → [buildfaster:p3][leave open]
Comment 24•12 years ago
|
||
It seems that this commit is causing the following failure here : 285:08.95 HTMLTableAccessible.cpp 285:30.68 Creating makedepend file .deps/xpcAccEvents.pp 285:30.69 Traceback (most recent call last): 285:30.69 File "/home/landry/src/mozilla-central/config/pythonpath.py", line 56, in <module> 285:30.80 main(sys.argv[1:]) 285:30.80 File "/home/landry/src/mozilla-central/config/pythonpath.py", line 48, in main 285:30.81 execfile(script, frozenglobals) 285:30.81 File "/home/landry/src/mozilla-central/accessible/src/xpcom/AccEventGen.py", line 240, in <module> 285:30.81 main() 285:30.81 File "/home/landry/src/mozilla-central/accessible/src/xpcom/AccEventGen.py", line 233, in main 285:30.81 makeutils.writeMakeDependOutput(options.makedepend_output) 285:30.81 File "/home/landry/src/mozilla-central/python/codegen/makeutils.py", line 13, in writeMakeDependOutput 285:30.85 with open(filename, 'w') as f: 285:30.85 IOError: [Errno 2] No such file or directory: '.deps/xpcAccEvents.pp' 285:30.88 gmake[7]: *** [xpcAccEvents.cpp] Error 1 285:30.88 gmake[7]: *** Deleting file `xpcAccEvents.cpp' Even with a clobber. $objdir/accessible/src/xpcom/.deps doesnt exist, if that's the expected dir....
Comment 25•12 years ago
|
||
Note: this was with mach. using regular client.mk seems to be okay : gmake[7]: Entering directory `/usr/obj/m-c/accessible/src/xpcom' /usr/obj/m-c/_virtualenv/bin/python /home/landry/src/mozilla-central/config/pythonpath.py \ -I/usr/obj/m-c/dist/sdk/bin \ /home/landry/src/mozilla-central/accessible/src/xpcom/AccEventGen.py \ -I ../../../dist/idl \ --header-output xpcAccEvents.h \ --stub-output xpcAccEvents.cpp \ --makedepend-output .deps/xpcAccEvents.pp \ /home/landry/src/mozilla-central/accessible/src/xpcom/AccEvents.conf Creating makedepend file .deps/xpcAccEvents.pp mkdir -p ".deps/" xpcAccEvents.cpp I guess it's related to EXTRA_MDDEPEND_FILES ?
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #25) > Note: this was with mach. using regular client.mk seems to be okay : > > gmake[7]: Entering directory `/usr/obj/m-c/accessible/src/xpcom' > /usr/obj/m-c/_virtualenv/bin/python > /home/landry/src/mozilla-central/config/pythonpath.py \ > -I/usr/obj/m-c/dist/sdk/bin \ > /home/landry/src/mozilla-central/accessible/src/xpcom/AccEventGen.py \ > -I ../../../dist/idl \ > --header-output xpcAccEvents.h \ > --stub-output xpcAccEvents.cpp \ > --makedepend-output .deps/xpcAccEvents.pp \ > /home/landry/src/mozilla-central/accessible/src/xpcom/AccEvents.conf > Creating makedepend file .deps/xpcAccEvents.pp > mkdir -p ".deps/" > xpcAccEvents.cpp > > > I guess it's related to EXTRA_MDDEPEND_FILES ? Can you file a followup bug?
Backed out due to bug 852249. https://hg.mozilla.org/integration/mozilla-inbound/rev/d764382ed4cf
Reporter | ||
Comment 28•12 years ago
|
||
Backed out because this made builds with Pymake way slower. bug 852249 covers fixing that as a prerequisite to re-landing this.
Comment 30•12 years ago
|
||
Given the pain removing mddepend.pl caused (by creating much larger make files that need to be evaluated), I'm wondering if keeping mddepend.pl isn't such a bad optimization after all.
Reporter | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [buildfaster:p3][leave open] → [buildfaster:p3]
Reporter | ||
Updated•12 years ago
|
Target Milestone: --- → mozilla23
We still need to land the other patches here, no?
Assignee | ||
Comment 35•12 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #34) > We still need to land the other patches here, no? Yes, one on c-c and the other on m-c after the c-c one lands. The c-c one probably needs an update with the same change you did to the one that landed.
Comment 36•12 years ago
|
||
(In reply to Ed Morley (Away Fri 12th) [:edmorley UTC+1] from comment #33) > https://hg.mozilla.org/mozilla-central/rev/260999a5d63b This patch adds the line of + $(PYTHON) $(topsrcdir)/build/unix/add_phony_targets.py $(_MDDEPFILE) ; \ But build/unix/add_phony_targets.py was backed out, not re-added.
Assignee | ||
Comment 37•12 years ago
|
||
c-c part: https://hg.mozilla.org/comm-central/rev/60e9936a1bdb missing add_phony_targets.py: https://hg.mozilla.org/integration/mozilla-inbound/rev/556eb9acc6b5 mddepend.pl removal: https://hg.mozilla.org/integration/mozilla-inbound/rev/a637a18d57bf
Comment 38•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/556eb9acc6b5 https://hg.mozilla.org/mozilla-central/rev/a637a18d57bf
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•