Closed Bug 692625 Opened 13 years ago Closed 13 years ago

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


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: emorley, Assigned: emorley)




(10 files)

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.
Attached file 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.
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/ to read "DIRS = bar/xyz", remove foo/bar/, and not have to move any files around.
(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.
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.
Attachment #578867 - Flags: review?(
Attachment #578868 - Flags: review?(
Attachment #578869 - Flags: review?(
Rob, moves browser/devtools/styleinspector/test/browser/ to browser/devtools/styleinspector/test/ since the parent directory is otherwise empty.
Attachment #578871 - Flags: review?(rcampbell)
Moves browser/devtools/webconsole/test/browser/ to browser/devtools/webconsole/test/
Attachment #578872 - Flags: review?(rcampbell)
Moves toolkit/components/aboutmemory/tests/chrome/ to toolkit/components/aboutmemory/tests/
Attachment #578873 - Flags: review?(nnethercote)
Moves toolkit/components/startup/tests/browser/ to toolkit/components/startup/tests/
Attachment #578874 - Flags: review?(dtownsend)
Attachment #578873 - Flags: review?(nnethercote) → review+
(standing rs=build, so not requesting review)
Attachment #578875 - Flags: review+
Attachment #578869 - Flags: review?( → review+
Attachment #578868 - Flags: review?( → review+
Attachment #578867 - Flags: review?( → review+
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 ***** */

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?
(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.
I've done it again!

(also remembering to add to CC this time)
Attachment #578874 - Flags: review?(dtownsend) → review+
Slight rebase + including the move of a few more tests added in those directories since:
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.