Flatten directories in the tree that only contain a single subdirectory and no files

RESOLVED FIXED in mozilla11


8 years ago
a year ago


(Reporter: emorley, Assigned: emorley)



Firefox Tracking Flags

(Not tracked)



(10 attachments)



8 years ago
Broken out from bug 689884 comment 12.

There exists in the tree a number of directories whose parent, only contains that directory and nothing else - and so could be flattened into the parent.

Comment 1

7 years ago
Created attachment 575760 [details]
Script to find such directories

Comment 2

7 years ago
Created attachment 575761 [details]
Raw results

Output of the script. Obviously contains a number of directories that will not presumably not want to be flattened (eg db/sqlite/ even though it's the only folder in db/), and will need to go through manually.

Comment 3

7 years ago
Dao, what are your thoughts about the results attachment? This bug was created on your suggestion (like you, I agree with it's intent), however I can see this turning into a nightmare of chasing module owners up and then people getting cross for weeks after, since none of their patch queues will then apply. 

Do you have any suggestions as to how best to go about this / roughly which of the types (eg tests folders that only contain one type of tests and so on) in the results file you see as worth being moved?
An alternative solution is to change the DIRS line for parent dirs. If you have foo/bar, which just has "DIRS = xyz", you could change foo/Makefile.in to read "DIRS = bar/xyz", remove foo/bar/Makefile.in, and not have to move any files around.

Comment 5

7 years ago
(In reply to Ted Mielczarek [:ted, :luser] from comment #4)
> An alternative solution is to change the DIRS line for parent dirs

That's what I did in bug 689884 (all instances should now be changed, unless anyone's added any since), however in that bug Dao suggested that I should have collapsed the directories instead (bug 689884 comment 12), which was why I filed this. 

However now that I've scanned the tree I'm somewhat indifferent, given the sheer number of directories (only some were as a result of bug 689884), and given that for many of them their existence is justified or else perhaps going to cause more upset than it's worth to move.

Comment 6

7 years ago
errr... and by Dao I of course meant Gavin throughout, whoops.

Sorry for the spam Dao.
I think these should be fixed:

I don't really care about the others.

Comment 8

7 years ago
Created attachment 578867 [details] [diff] [review]
A: browser/components/sessionstore/test
Attachment #578867 - Flags: review?(gavin.sharp)

Comment 9

7 years ago
Created attachment 578868 [details] [diff] [review]
B: browser/components/sidebar
Attachment #578868 - Flags: review?(gavin.sharp)

Comment 10

7 years ago
Created attachment 578869 [details] [diff] [review]
C: browser/components/test
Attachment #578869 - Flags: review?(gavin.sharp)

Comment 11

7 years ago
Created attachment 578871 [details] [diff] [review]
D: browser/devtools/styleinspector/test

Rob, moves browser/devtools/styleinspector/test/browser/ to browser/devtools/styleinspector/test/ since the parent directory is otherwise empty.
Attachment #578871 - Flags: review?(rcampbell)

Comment 12

7 years ago
Created attachment 578872 [details] [diff] [review]
E: browser/devtools/webconsole/test

Moves browser/devtools/webconsole/test/browser/ to browser/devtools/webconsole/test/
Attachment #578872 - Flags: review?(rcampbell)

Comment 13

7 years ago
Created attachment 578873 [details] [diff] [review]
F: toolkit/components/aboutmemory/tests

Moves toolkit/components/aboutmemory/tests/chrome/ to toolkit/components/aboutmemory/tests/
Attachment #578873 - Flags: review?(nnethercote)

Comment 14

7 years ago
Created attachment 578874 [details] [diff] [review]
G: toolkit/components/startup/tests

Moves toolkit/components/startup/tests/browser/ to toolkit/components/startup/tests/
Attachment #578874 - Flags: review?(dtownsend)
Attachment #578873 - Flags: review?(nnethercote) → review+

Comment 15

7 years ago
Created attachment 578875 [details] [diff] [review]
H: Update *makefiles.sh to reflect new paths

(standing rs=build, so not requesting review)
Attachment #578875 - Flags: review+
Attachment #578869 - Flags: review?(gavin.sharp) → review+
Attachment #578868 - Flags: review?(gavin.sharp) → review+
Attachment #578867 - Flags: review?(gavin.sharp) → review+

Comment 16

7 years ago
Meant to add: Everything passes try at the moment, and when reviews are complete, I'll be landing over a weekend whilst it's quiet, so I can tryserver again + land straight after, before other things can break it. (Given how unsuccessfully mercurial backs out renames, I want to make 110% sure this sticks).
Comment on attachment 578873 [details] [diff] [review]
F: toolkit/components/aboutmemory/tests

Review of attachment 578873 [details] [diff] [review]:

Looks like gavin already r+'d, but this seems fine to me -- I just copied the structure from somewhere else so I'm not greatly attached to it.
Comment on attachment 578871 [details] [diff] [review]
D: browser/devtools/styleinspector/test

looks ok.
Attachment #578871 - Flags: review?(rcampbell) → review+
Comment on attachment 578872 [details] [diff] [review]
E: browser/devtools/webconsole/test


--- a/browser/devtools/webconsole/test/browser/browser_warn_user_about_replaced_api.js
+++ b/browser/devtools/webconsole/test/browser_warn_user_about_replaced_api.js
@@ -32,17 +32,17 @@
  * use your version of this file under the terms of the MPL, indicate your
  * decision by deleting the provisions above and replace them with the notice
  * and other provisions required by the GPL or the LGPL. If you do not delete
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  * ***** END LICENSE BLOCK ***** */
-const TEST_REPLACED_API_URI = "http://example.com/browser/browser/devtools/webconsole/test//browser/test-console-replaced-api.html";
+const TEST_REPLACED_API_URI = "http://example.com/browser/browser/devtools/webconsole/test//test-console-replaced-api.html";

Extra '/' character in that URL. Should probably fix that.

another one of these in:

+++ b/browser/devtools/webconsole/test/browser_webconsole_basic_net_logging.js

I'm wondering if our previous move script was broken because all of these URI consts seem to have them.

Since they were like that when you got them, I'll R+ this with a follow-up to clean up the URIs.

Thanks for doing this!
Attachment #578872 - Flags: review?(rcampbell) → review+
filed bug 707650 to clean up the extra characters in the webconsole tests.
Comment on attachment 578874 [details] [diff] [review]
G: toolkit/components/startup/tests

Review of attachment 578874 [details] [diff] [review]:

What is the benefit of this move?

Comment 22

7 years ago
(In reply to Dave Townsend (:Mossop) from comment #21)
> What is the benefit of this move?

Reducing unnecessary hierarchy depth in the trees + Dao asked that I do it in bug 689884 comment 12. Happy for you and Dao to come to an agreement either way and obsolete this part's patch if decided easiest.

Comment 23

7 years ago
I've done it again!

(also remembering to add to CC this time)
Attachment #578874 - Flags: review?(dtownsend) → review+

Comment 24

7 years ago
Slight rebase + including the move of a few more tests added in those directories since:


a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.