689884 - Remove no-op makefiles & combine those that only |DIRS = a_single_subdirectory|

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ed Morley [:emorley]]

Attachment: Script for finding potentially unnecessary makefiles

There exists in the tree a number of Makefiles that either do nothing (ie are empty apart from the boilerplate and whose directory doesn't contain a jar.mn) or else just contain a single |DIRS = subdirectory|, which could just be moved to the Makefile one level higher up.

The attached script is fairly crude (hence the whitelist), but attempts to generate a list of these unnecessary Makefiles. 

When run from topsrcdir, it will output the relative path of makefiles that match all of the following:
 ~ Are not on the whitelist
 ~ Whose directory does not contain a jar.mn
 ~ Do not contain more than one DIR(S) string
 ~ Do not contain any of the strings in the ignore pattern.

Comment 1

[Ed Morley [:emorley]]

Attachment: Part 1: Makefile changes

- Remove references to no-op Makefiles.
- If a Makefile only |DIRS = a_single_subdirectory|, include the subdirectory directly, rather than the superfluous parent Makefile.

Parts 1-3 have passed try:
https://tbpl.mozilla.org/?tree=Try&rev=a43a3daadaaa

Comment 2

[Ed Morley [:emorley]]

Attachment: Part 2: Rm the now unused Makefiles

Comment 3

[Ed Morley [:emorley]]

Attachment: Part 3: Adjust *makefiles.sh

Comment 4

[Matheus Kerschbaum]

Great work!

Comment 5

[Joey Armstrong [:joey]]

edmorely++

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 563019 [details] [diff] [review]
Part 1: Makefile changes

Review of attachment 563019 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/Makefile.in
@@ +45,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  include $(topsrcdir)/config/config.mk
>  
>  DIRS = \

/me idly wonders if these could be PARALLEL_DIRS

::: browser/devtools/styleinspector/Makefile.in
@@ +44,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  ifdef ENABLE_TESTS
>  ifneq (mobile,$(MOZ_BUILD_APP))
> +	DIRS += test/browser

kill the tabs!

@@ +51,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
>  
>  libs::
> +	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools

I stared at this for five minutes without figuring out what changed on this line.  What's different?

::: browser/devtools/webconsole/Makefile.in
@@ +56,5 @@
>  		$(NULL)
>  
>  ifdef ENABLE_TESTS
>  ifneq (mobile,$(MOZ_BUILD_APP))
> +	DIRS += test/browser

No tabs!

::: caps/Makefile.in
@@ +45,5 @@
>  MODULE		= caps
>  DIRS		= idl include src
>  
>  ifdef ENABLE_TESTS
> +DIRS		+= tests/mochitest

Kill the tabs!

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 563020 [details] [diff] [review]
Part 2: Rm the now unused Makefiles

I'm just assuming this is correct.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 563021 [details] [diff] [review]
Part 3: Adjust *makefiles.sh

Review of attachment 563021 [details] [diff] [review]:
-----------------------------------------------------------------

And likewise.

Comment 9

[Ed Morley [:emorley]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> > +	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools
> 
> I stared at this for five minutes without figuring out what changed on this
> line.  What's different?

Fixing no newline at end of file, sorry for the confusion, I should have mentioned it in the upload comment :-)

I'll sort out the rest when I get back - thanks for the fast review!

Comment 10

[Ed Morley [:emorley]]

Attachment: Part 1: Makefile changes v1.1

As before, but with tabs removed whilst I'm there, per comment 6.

Comment 11

[Ed Morley [:emorley]]

Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4cb3b08fa1
Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/d04362b1e256
Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/26f668dc53fb

Comment 12

[:Gavin Sharp [email: [e-mail]]]

I busted inbound with a bad merge on top of this patch. Rather than only changing the makefiles, you should have flattened browser/components/sidebar/src into browser/components/sidebar. I did that in a followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8af101010da7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8abcac1d00a

Are there other similar cases in that patch? We should fix those too.

Comment 13

[Ed Morley [:emorley]]

I'm not quite sure why the inbound bustage fix due to bug 692130 rebase conflicts needed to be attributed to this bug in the commit message; but thank you for folding browser/components/sidebar/src/ into sidebar/ in the process :-)

I considered flattening when filing this bug, but it seemed presumptuous to change the folder layout of other people's modules (when I had no idea as to what their plans were for future files in that component), so decided against it. In addition, the majority of the other makefiles removed, were for directories that contained additional folders, so not suited to flattening anyway.

I've filed bug 692625 for checking for other directories in the tree that could be similarly flattened, either as a result of the patches here, or else from before them.

Thanks!

Comment 14

[Ed Morley [:emorley]]

For those merging, 94f2fa9f97b8 has also been attributed to this bug in the commit message, but the correct bug is bug 692130.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Ed Morley [:edmorley] from comment #13)
> I considered flattening when filing this bug, but it seemed presumptuous to
> change the folder layout of other people's modules (when I had no idea as to
> what their plans were for future files in that component)

No less presumptuous than changing around their makefiles, really - something like this should get signoff from the relevant stakeholders (this one would have been an easy signoff to give).

Thanks for dealing with my bustage on inbound, I was under a time crunch and had to leave unfortunately :(

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ec4cb3b08fa1
https://hg.mozilla.org/mozilla-central/rev/d04362b1e256
https://hg.mozilla.org/mozilla-central/rev/26f668dc53fb

(The merge revs for those in comment 12 and comment 14 have been pasted in bug 692130)

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Moving files around also makes blame harder to use, which some people are quite sensitive to.

Changing makefiles is far less "presumptuous" than moving other people's code around.  Makefiles belong to the build system, review by module peers of the modules they build in is not required.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #17)
> Changing makefiles is far less "presumptuous" than moving other people's
> code around.  Makefiles belong to the build system, review by module peers
> of the modules they build in is not required.

I disagree.