make dependencies smarter, get rid of mddepend.pl

RESOLVED FIXED in mozilla23

Status

()

Core
Build Config
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: ted, Assigned: glandium)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [buildfaster:p3], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
I've got plane-time to look at this: should be simple!
Assignee: nobody → benjamin
(Reporter)

Comment 2

9 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

9 years ago
Created attachment 346325 [details] [diff] [review]
mddepend removal, rev. 1

This should be ready for prime-time
Attachment #346325 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 4

9 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

9 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

9 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

9 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

9 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

9 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

8 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

6 years ago
Blocks: 755684

Updated

6 years ago
Whiteboard: [buildfaster:?]
(Assignee)

Updated

5 years ago
Assignee: nobody → mh+mozilla
Depends on: 785622
(Assignee)

Comment 13

5 years ago
Created attachment 655332 [details] [diff] [review]
Get rid of mddepend.pl

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

5 years ago
Whiteboard: [buildfaster:?] → [buildfaster:p3]
(Assignee)

Comment 14

5 years ago
Created attachment 724468 [details] [diff] [review]
Stop using mddepend.pl

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

5 years ago
Attachment #655332 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 724469 [details] [diff] [review]
Remove mddepend.pl
Attachment #724469 - Flags: review?(ted)
(Assignee)

Comment 16

5 years ago
Created attachment 724471 [details] [diff] [review]
Stop using mddepend.pl

with config/rules.mk in sync.
Attachment #724471 - Flags: review?(ted)
Attachment #724471 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #724468 - Attachment is obsolete: true
Attachment #724468 - Flags: review?(ted)
Attachment #724468 - Flags: review?(gps)
(Assignee)

Comment 17

5 years ago
Created attachment 724473 [details] [diff] [review]
Stop using mddepend.pl (c-c)
Attachment #724473 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

5 years ago
Attachment #346325 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #724469 - Flags: review?(ted) → review+
(Reporter)

Comment 18

5 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

5 years ago
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+
(Assignee)

Comment 21

5 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

5 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]
https://hg.mozilla.org/mozilla-central/rev/fc11223a7745
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 ?
(Assignee)

Comment 26

5 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?
(Reporter)

Updated

5 years ago
Depends on: 852249
Backed out due to bug 852249.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d764382ed4cf
(Reporter)

Comment 28

5 years ago
Backed out because this made builds with Pymake way slower. bug 852249 covers fixing that as a prerequisite to re-landing this.
Blocks: 851908
https://hg.mozilla.org/mozilla-central/rev/d764382ed4cf
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.
Created attachment 735265 [details] [diff] [review]
Stop using mddepend.pl

Updated to deal with pymake.
Attachment #724471 - Attachment is obsolete: true
http://hg.mozilla.org/integration/mozilla-inbound/rev/260999a5d63b
https://hg.mozilla.org/mozilla-central/rev/260999a5d63b
(Reporter)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [buildfaster:p3][leave open] → [buildfaster:p3]
(Reporter)

Updated

5 years ago
Target Milestone: --- → mozilla23
We still need to land the other patches here, no?
(Assignee)

Comment 35

5 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

5 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

5 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
https://hg.mozilla.org/mozilla-central/rev/556eb9acc6b5
https://hg.mozilla.org/mozilla-central/rev/a637a18d57bf
You need to log in before you can comment on or make changes to this bug.