761138 - [OS.File] Create / remove empty directories

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, enhancement)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

After some tweaking around, I have come to the conclusion that we do not want to build recursive directory removal into OS.File yet. The main reason for this is that, depending on some information that OS.File does not have, we can have several very distinct implementations:
- move the directory to the OS-specific trash;
- move the directory to the OS-specific temporary directory;
- walk through the directory recursively, removing files as they come;
- walk through the directory recursively but asynchronously, removing a maximal number of files at a time, with a delay before two chunks;
- one of the two above combinations, but first move the directory to a kill-directory, that also needs to be emptied on startup;
- ...

For the moment, I therefore intend to only provide the features of moving directories/removing empty directories/walking through hierarchies. We will see for more advanced features once I have a clearer idea of the situation.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. The test suite

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: 7. Constants

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Removing an empty directory (Unix)

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Removing an empty directory (Windows)

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. Creating a directory (Unix)

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: 5. Creating a directory (Windows back-end)

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: 6. Creating a directory (Windows front-end)

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Windows

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Unix

Comment 11

[David Teller [:Yoric] - still alive but not very active]

I have slightly changed my roadmap. Unless someone explicitly needs this, I will land this after bug 729057.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Windows

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Unix

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: The constants

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 652432 [details] [diff] [review]
mkdir/rmdir for Windows

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

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +328,5 @@
> +      *     directory does not exist yet.
> +      */
> +     File.removeEmptyDir = function removeEmptyDir(path, options) {
> +       options = options || noOptions;
> +       let result = UnixFile.rmdir(path);

Copy-paste-o?

@@ +346,5 @@
> +      * @param {*=} options Additional options. This
> +      * implementation interprets the following fields:
> +      *
> +      * - {number} winSecurity If specified, security attributes
> +      * as per winapi function |CreateDirectory|. If unspecified,

Wait, what?  The function takes a void* for security parameters and we merrily document that it takes numbers?  That seems mistaken.  The documentation should also be referring to |CreateDirectoryW|, no?

Comment 17

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #16)
> Comment on attachment 652432 [details] [diff] [review]
> mkdir/rmdir for Windows
> 
> Review of attachment 652432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_win_front.jsm
> @@ +328,5 @@
> > +      *     directory does not exist yet.
> > +      */
> > +     File.removeEmptyDir = function removeEmptyDir(path, options) {
> > +       options = options || noOptions;
> > +       let result = UnixFile.rmdir(path);
> 
> Copy-paste-o?

Well-spotted, thanks.

> 
> @@ +346,5 @@
> > +      * @param {*=} options Additional options. This
> > +      * implementation interprets the following fields:
> > +      *
> > +      * - {number} winSecurity If specified, security attributes
> > +      * as per winapi function |CreateDirectory|. If unspecified,
> 
> Wait, what?  The function takes a void* for security parameters and we
> merrily document that it takes numbers?  That seems mistaken.

Documentation error, indeed.

> The
> documentation should also be referring to |CreateDirectoryW|, no?

Well, officially, according to Microsoft, the function is called |CreateDirectory|. The fact that it is #defined to |CreateDirectoryW| is an implementation trick that I do not want to document.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Windows

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller from comment #17)
> > @@ +346,5 @@
> > > +      * @param {*=} options Additional options. This
> > > +      * implementation interprets the following fields:
> > > +      *
> > > +      * - {number} winSecurity If specified, security attributes
> > > +      * as per winapi function |CreateDirectory|. If unspecified,
> > 
> > Wait, what?  The function takes a void* for security parameters and we
> > merrily document that it takes numbers?  That seems mistaken.
> 
> Documentation error, indeed.

Neither here nor there, but do we really have a standardized way of getting alternative security attributes to pass to the function?  If the answer is "no" or "do it via ctypes", that's ok.

> > The
> > documentation should also be referring to |CreateDirectoryW|, no?
> 
> Well, officially, according to Microsoft, the function is called
> |CreateDirectory|. The fact that it is #defined to |CreateDirectoryW| is an
> implementation trick that I do not want to document.

A pox on Microsoft's liberal use of #define'ing functions and whatnot in their headers.  Carry on.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #19)
> (In reply to David Rajchenbach Teller from comment #17)
> > > @@ +346,5 @@
> > > > +      * @param {*=} options Additional options. This
> > > > +      * implementation interprets the following fields:
> > > > +      *
> > > > +      * - {number} winSecurity If specified, security attributes
> > > > +      * as per winapi function |CreateDirectory|. If unspecified,
> > > 
> > > Wait, what?  The function takes a void* for security parameters and we
> > > merrily document that it takes numbers?  That seems mistaken.
> > 
> > Documentation error, indeed.
> 
> Neither here nor there, but do we really have a standardized way of getting
> alternative security attributes to pass to the function?  If the answer is
> "no" or "do it via ctypes", that's ok.

For the moment, "do it via ctypes". Adding this at a later stage should be possible, if it becomes necessary.

> 
> > > The
> > > documentation should also be referring to |CreateDirectoryW|, no?
> > 
> > Well, officially, according to Microsoft, the function is called
> > |CreateDirectory|. The fact that it is #defined to |CreateDirectoryW| is an
> > implementation trick that I do not want to document.
> 
> A pox on Microsoft's liberal use of #define'ing functions and whatnot in
> their headers.  Carry on.

Yeah, multiple poxes on winAPI for just about everything. And don't get me started on paths :)

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 652437 [details] [diff] [review]
Companion testsuite

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +622,5 @@
> +{
> +  ok(true, "test_mkdir: Starting");
> +
> +  let dirName = "test_dir.tmp";
> +  OS.File.removeEmptyDir(dirName, {ignoreAbsent: true});

Checking that you throw for not-present ignoreAbsent in this case would be a good idea.

@@ +626,5 @@
> +  OS.File.removeEmptyDir(dirName, {ignoreAbsent: true});
> +
> +  ok(true, "test_mkdir: Creating directory");
> +  OS.File.makeDir(dirName);
> +  ok(true, OS.File.stat(dirName).isDir, "test_mkdir: Created directory is a directory");

What do you think about checking makeDir(dir) when dir already exists and/or is a file?

@@ +634,5 @@
> +  try {
> +    OS.File.stat(dirName);
> +    ok(false, "test_mkdir: Directory was not removed");
> +  } catch (x) {
> +    ok(x instanceof OS.File.Error && x.becauseNoSuchFile, "test_mkdir: Directory was correcly removed");

Just "test_mkdir: Directory was removed"; shorter and fixes the typo. :)

@@ +636,5 @@
> +    ok(false, "test_mkdir: Directory was not removed");
> +  } catch (x) {
> +    ok(x instanceof OS.File.Error && x.becauseNoSuchFile, "test_mkdir: Directory was correcly removed");
> +  }
> +

Checking that you throw for removing a non-empty directory would be good.  Checking that you throw for rmdir'ing a file would be good as well.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite, v2

Here we go

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 652732 [details] [diff] [review]
Companion testsuite, v2

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

r=me with the changes below.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +627,5 @@
> +
> +  // Check that removing absent directories is handled correctly
> +  let exn;
> +  try {
> +    OS.File.removeEmptyDir(dirName, {ignoreAbsent: true});

This looks like it's duplicating the test 5 lines earlier.  Why not just use the earlier one for the test or delete the earlier one?

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Unix

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Windows

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Windows

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Attachment: The constants

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Looks like this is failing tests on Windows.
https://tbpl.mozilla.org/?tree=Try&rev=e0fee2e553ca

https://tbpl.mozilla.org/php/getParsedLog.php?id=14590527&tree=Try

13620 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | test_mkdir: Creating directory that already exists
13621 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | test_mkdir: makeDir over an existing directory failed for all the right reasons
13622 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | test_mkdir: Directory was removed

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Investigating

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: The constants, v2

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Attachment: mkdir/rmdir for Windows

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Green on Try. Sorry for the obscenely long delay.
https://tbpl.mozilla.org/?tree=Try&rev=c464f2934ab8

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b8fb3c9395
https://hg.mozilla.org/integration/mozilla-inbound/rev/d94e6c20c4dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb65ac5a609
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd86c175258

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9b8fb3c9395
https://hg.mozilla.org/mozilla-central/rev/d94e6c20c4dc
https://hg.mozilla.org/mozilla-central/rev/3eb65ac5a609
https://hg.mozilla.org/mozilla-central/rev/1fd86c175258