The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

Trunk
mozilla10
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.
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
Attachment #563019 - Flags: review?(khuey)
Created attachment 563020 [details] [diff] [review]
Part 2: Rm the now unused Makefiles
Attachment #563020 - Flags: review?(khuey)
Created attachment 563021 [details] [diff] [review]
Part 3: Adjust *makefiles.sh
Attachment #563021 - Flags: review?(khuey)
Summary: Remove/combine makefiles that either do nothing or else only |DIRS = a_single_subdirectory| → Remove no-op makefiles & combine those that only |DIRS = a_single_subdirectory|

Comment 4

6 years ago
Great work!
edmorely++
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!
Attachment #563019 - Flags: review?(khuey) → review+
Comment on attachment 563020 [details] [diff] [review]
Part 2: Rm the now unused Makefiles

I'm just assuming this is correct.
Attachment #563020 - Flags: review?(khuey) → review+
Comment on attachment 563021 [details] [diff] [review]
Part 3: Adjust *makefiles.sh

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

And likewise.
Attachment #563021 - Flags: review?(khuey) → review+
(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!
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.
Attachment #563019 - Attachment is obsolete: true
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
Flags: in-testsuite-
Target Milestone: --- → mozilla10
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.
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!
For those merging, 94f2fa9f97b8 has also been attributed to this bug in the commit message, but the correct bug is bug 692130.
(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 :(
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.
(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.
You need to log in before you can comment on or make changes to this bug.