Last Comment Bug 689884 - Remove no-op makefiles & combine those that only |DIRS = a_single_subdirectory|
: Remove no-op makefiles & combine those that only |DIRS = a_single_subdirectory|
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Ed Morley [:emorley]
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-28 02:29 PDT by Ed Morley [:emorley]
Modified: 2011-10-07 14:28 PDT (History)
5 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Script for finding potentially unnecessary makefiles (2.55 KB, text/plain)
2011-09-28 02:29 PDT, Ed Morley [:emorley]
no flags Details
Part 1: Makefile changes (10.99 KB, patch)
2011-09-28 03:33 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review
Part 2: Rm the now unused Makefiles (53.62 KB, patch)
2011-09-28 03:34 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review
Part 3: Adjust *makefiles.sh (9.18 KB, patch)
2011-09-28 03:35 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review
Part 1: Makefile changes v1.1 (10.96 KB, patch)
2011-10-06 05:09 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2011-09-28 02:29:20 PDT
Created attachment 563010 [details]
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] 2011-09-28 03:33:09 PDT
Created attachment 563019 [details] [diff] [review]
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] 2011-09-28 03:34:35 PDT
Created attachment 563020 [details] [diff] [review]
Part 2: Rm the now unused Makefiles
Comment 3 Ed Morley [:emorley] 2011-09-28 03:35:49 PDT
Created attachment 563021 [details] [diff] [review]
Part 3: Adjust *makefiles.sh
Comment 4 Matheus Kerschbaum 2011-09-28 14:20:26 PDT
Great work!
Comment 5 Joey Armstrong [:joey] 2011-09-28 16:29:57 PDT
edmorely++
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-30 05:30:47 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-30 05:31:12 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-30 05:32:42 PDT
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] 2011-10-01 03:39:34 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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] 2011-10-06 05:09:20 PDT
Created attachment 565189 [details] [diff] [review]
Part 1: Makefile changes v1.1

As before, but with tabs removed whilst I'm there, per comment 6.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-06 14:37:26 PDT
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] 2011-10-06 15:36:04 PDT
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] 2011-10-06 15:47:18 PDT
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: gavin@gavinsharp.com] 2011-10-06 18:57:14 PDT
(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 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-07 04:39:15 PDT
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: gavin@gavinsharp.com] 2011-10-07 14:28:59 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

Note You need to log in before you can comment on or make changes to this bug.