(not currently active) Ted Mielczarek Reporter
	Description • 16 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:

	Benjamin Smedberg
	Comment 1 • 16 years ago

I've got plane-time to look at this: should be simple!

	(not currently active) Ted Mielczarek 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.

	Benjamin Smedberg
	Comment 3 • 16 years ago

This should be ready for prime-time

	(not currently active) 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.

	Benjamin Smedberg
	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.

	(not currently active) Ted Mielczarek 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

	(not currently active) Ted Mielczarek 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.

	(not currently active) Ted Mielczarek 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.

	Benjamin Smedberg
	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...

	(not currently active) Ted Mielczarek 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. "

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 11 • 14 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 12 • 14 years ago

Tentatively taking, the WIP I have in my queue for 623617 does this.

	Mike Hommey [:glandium] 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.

	Mike Hommey [:glandium] Assignee
	Comment 14 • 12 years ago

Ted for the general thing, gps for the python script that uses pymake's API.

	Mike Hommey [:glandium] Assignee
	Comment 16 • 12 years ago

with config/rules.mk in sync.

	(not currently active) Ted Mielczarek 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...

	Mark Banner (:standard8)
	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.

	Gregory Szorc [:gps]
	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.

	Mike Hommey [:glandium] 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 :)

	Mike Hommey [:glandium] 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.

	Phil Ringnalda (:philor)
	Comment 23 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/fc11223a7745

	Landry Breuil (:gaston)
	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....

	Landry Breuil (:gaston)
	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 ?

	Mike Hommey [:glandium] 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?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 27 • 12 years ago

Backed out due to bug 852249.

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

	(not currently active) Ted Mielczarek 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/d764382ed4cf

	Gregory Szorc [:gps]
	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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 31 • 12 years ago

Updated to deal with pymake.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 32 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/260999a5d63b

	Ed Morley [:emorley]
	Comment 33 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/260999a5d63b

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 34 • 12 years ago

We still need to land the other patches here, no?

	Mike Hommey [:glandium] 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.

	Ginn Chen
	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.

	Mike Hommey [:glandium] 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

	Phil Ringnalda (:philor)
	Comment 38 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/556eb9acc6b5
https://hg.mozilla.org/mozilla-central/rev/a637a18d57bf