Closed Bug 462463 Opened 11 years ago Closed 7 years ago

make dependencies smarter, get rid of mddepend.pl

Categories

(Firefox Build System :: General, defect)

defect
Not set

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)

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:
I've got plane-time to look at this: should be simple!
Assignee: nobody → benjamin
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.
Attached patch mddepend removal, rev. 1 (obsolete) — Splinter Review
This should be ready for prime-time
Attachment #346325 - Flags: review?(ted.mielczarek)
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.
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.
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
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.
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+
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...
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
Blocks: 755684
Whiteboard: [buildfaster:?]
Assignee: nobody → mh+mozilla
Depends on: 785622
Attached patch Get rid of mddepend.pl (obsolete) — Splinter Review
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.
Whiteboard: [buildfaster:?] → [buildfaster:p3]
Attached patch Stop using mddepend.pl (obsolete) — Splinter Review
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)
Attachment #655332 - Attachment is obsolete: true
Attachment #724469 - Flags: review?(ted)
Attached patch Stop using mddepend.pl (obsolete) — Splinter Review
with config/rules.mk in sync.
Attachment #724471 - Flags: review?(ted)
Attachment #724471 - Flags: review?(gps)
Attachment #724468 - Attachment is obsolete: true
Attachment #724468 - Flags: review?(ted)
Attachment #724468 - Flags: review?(gps)
Attachment #724473 - Flags: review?(bugspam.Callek)
Attachment #346325 - Attachment is obsolete: true
Attachment #724469 - Flags: review?(ted) → review+
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+
Attachment #724473 - Flags: review?(mbanner)
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 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+
(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 :)
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]
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....
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 ?
(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?
Depends on: 852249
Backed out because this made builds with Pymake way slower. bug 852249 covers fixing that as a prerequisite to re-landing this.
Blocks: 851908
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.
Updated to deal with pymake.
Attachment #724471 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [buildfaster:p3][leave open] → [buildfaster:p3]
Target Milestone: --- → mozilla23
We still need to land the other patches here, no?
(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.
(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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.