934283 - Add option to OS.File.makeDir to recursively make directories

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

bug 923540 added an API to recursivwely remove directories - it would be very useful if OS.File.makeDir allowed recursively making them. This would really simplify code and error handling some some areas of code.

Comment 1

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

Attachment: Creating directories and their ancestors

Comment 2

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

This implementation is fully cross-platform.
Mike, in your experience, do you believe that we should investigate a Linux-specific implementation that would use |openat| + |mkdirat| to possibly speed up things?

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #2)
> This implementation is fully cross-platform.
> Mike, in your experience, do you believe that we should investigate a
> Linux-specific implementation that would use |openat| + |mkdirat| to
> possibly speed up things?

Is that the rationale behind the |from| option in the current code?  (It seems to be a really odd way of simulating |mkdir -p| otherwise.)

Comment 4

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

(In reply to Nathan Froyd (:froydnj) (AFK 23-30 November) from comment #3)
> Is that the rationale behind the |from| option in the current code?  (It
> seems to be a really odd way of simulating |mkdir -p| otherwise.)

Not really.
The idea is that I can see the following strategies:
1. start from root, check if each component exist, once they stop existing, attempt to create the missing components;
2. start from leaf, check if each component exist, once they start existing, attempt to create the missing components;
3. weird variants involving e.g. dichotomy.

Strategies 1. and 2. are unsatisfying because we can have lots of (possibly non-trivial) I/O calls. In particular, most of the files we need to create are in the profileDir, which we know exists, so we might as well take advantage of this knowledge.

Strategy 3. is weird and complicated.

So I introduced strategy 4 ("user knows best"), which is simple and generally the best performing.

Comment 5

[Mike Hommey [:glandium]]

Unless you expect a script is going to require creating thousands of directories very quickly, i doubt there is any value optimizing for linux.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8337403 [details] [diff] [review]
Creating directories and their ancestors

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

r=me with more tests.  Your choice whether you want re-review.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +502,5 @@
> + *
> + * - {string} from If specified, the call to |makeDir| creates all the
> + * ancestors of |path| that are descendants of |from|. Note that |from|
> + * and its existing descendants must be user-writeable and that |path|
> + * must be a descendant of |from|.

I think you want to say something like:

"Note that |from| must be user-writable and that |path| must be a descendant of from.  Any subdirectories of |from| present in |path| must also be user-writable."

This makes it a little more clear than the "existing descendants" of |from| are found in |path| and don't just spring up out of nowhere.  Further improvements welcome.

::: toolkit/components/osfile/tests/xpcshell/test_makeDir.js
@@ +68,5 @@
> +
> +/**
> + * Creating subdirectories
> + */
> +add_task(function option_from() {

You need to check whether creating a path that's not prefixed by |from| fails.

You need to check whether creating a previously non-existent path without |from| succeeds.

A test that tries to create a path where some components "in the middle" are not writable would be good.

Comment 7

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

(In reply to Nathan Froyd (:froydnj) from comment #6)
> A test that tries to create a path where some components "in the middle" are
> not writable would be good.

Unfortunately, I we do not have a setMode function yet, so that's probably not possible atm.

Comment 8

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

Attachment: Creating directories and their ancestors, v2

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=7541311356df

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/083cbb15c2a5

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/083cbb15c2a5

Comment 12

[noitidart]

Hi all,
I was using makeDir to allow users of my addon to set a path for a non-relative profile. So like they can provide `var tp = "C:\foo\bar\profile_name"`. So the folders `foo` and `bar` may not exist. `profile_name` definitely doesn't exist. I was planning on running `OS.File.makeDir(OS.Path.normalize(tp))`. This will work once this bug fix lands.

But can you please recommend the preferred way to handle this pre-fix?

Thanks!

Comment 13

[noitidart]

What i mean by pre-fix is like for browsers pre-ff31 i need to support ff29+

Comment 14

[noitidart]

Also how to make recursive from the start, why need to define a path in from? Why not just set from to true?

let promise = OS.File.makeDir(OS.Path.join('C:','Users','Vayeate','Desktop','new fol 1','new fold 2', 'my new profile folder'), {
     from: OS.Path.join('C:'),
    ignoreExisting: false
});

Here in this example I know C: but in other examples I wont know. Im just doing a blind makeDir on a path.

Comment 15

[noitidart]

Forgive the spamming up of the board. I got a new development on the `from` issue. Bugzilla really needs an edit feature :(

Anyways on linux, mac, and on windows for a path like: `\\rawr\\rawr\\my new profile` its not possible to make recursively :(


I detect with OS.Path.split if the path is absolute, and if it is then I do makeDir on it to make a `IsRelative=0` (so non-relative so absolute) profile folder.

var tp = '\\rawr\\rawr\\my new profile' //var typedPath
var tpSplit = OS.Path.split(tp);
//let promise = OS.File.makeDir(tp);

let promise = OS.File.makeDir(tp, {
   from: tpSplit.components[0]
});

This code throws this error on windows:

```
"makeDir failed, aRejectReason = " Object { operation: "makeDir", path: "\rawr\rawr\my new profile", winLastError: 3, stack: "", fileName: "", lineNumber: 0 } Scratchpad/3:17
"aRejectReason.becauseAccessDenied = " false Scratchpad/3:18
"aRejectReason.becauseClosed = " false Scratchpad/3:19
"aRejectReason.becauseExists = " false Scratchpad/3:20
"aRejectReason.becauseInvalidArgument = " false Scratchpad/3:21
"aRejectReason.becauseNoSuchFile = " true Scratchpad/3:22
"aRejectReason.becauseNotEmpty = " false Scratchpad/3:23
"makeDir failed, aRejectReason = " "Win error 3 during operation makeDir on file \rawr\rawr\my new profile (The system cannot find the path specified.

)" Scratchpad/3:24
"makeDir failed, aRejectReason = " Object { exn: "OS.File.Error", fileName: undefined, lineNumber: 0, stack: undefined, operation: "makeDir", winLastError: 3, path: "\rawr\rawr\my new profile" }
```

Comment 16

[noitidart]

To support pre ff31 I wrote this, works exactly the same as makeDir, it uses makeDir, just changes stuff a bit when from key exists: Copy paste example: https://gist.github.com/Noitidart/67a81d730ec53040189c

Here's just the function:


function makeDir_Bug934283(path, options) {
	// pre FF31, using the `from` option would not work, so this fixes that so users on FF 29 and 30 can still use my addon
	// the `from` option should be a string of a folder that you know exists for sure. then the dirs after that, in path will be created
	// for example: path should be: `OS.Path.join('C:', 'thisDirExistsForSure', 'may exist', 'may exist2')`, and `from` should be `OS.Path.join('C:', 'thisDirExistsForSure')`

	if (!('from' in options)) {
		throw new Error('you have no need to use this, as this is meant to allow creation from a folder that you know for sure exists');
	}

	if (path.toLowerCase().indexOf(options.from.toLowerCase()) == -1) {
		throw new Error('The `from` string was not found in `path` string');
	}

	var options_from = options.from;
	delete options.from;

	var dirsToMake = OS.Path.split(path).components.slice(OS.Path.split(options_from).components.length);
	console.log('dirsToMake:', dirsToMake);

	var deferred_makeDir_Bug934283 = new Deferred();
	var promise_makeDir_Bug934283 = deferred_makeDir_Bug934283.promise;

	var pathExistsForCertain = options_from;
	var makeDirRecurse = function() {
		pathExistsForCertain = OS.Path.join(pathExistsForCertain, dirsToMake[0]);
		dirsToMake.splice(0, 1);
		var promise_makeDir = OS.File.makeDir(pathExistsForCertain);
		promise_makeDir.then(
			function(aVal) {
				console.log('Fullfilled - promise_makeDir - ', 'ensured/just made:', pathExistsForCertain, aVal);
				if (dirsToMake.length > 0) {
					makeDirRecurse();
				} else {
					deferred_makeDir_Bug934283.resolve('this path now exists for sure: "' + pathExistsForCertain + '"');
				}
			},
			function(aReason) {
				var rejObj = {
					promiseName: 'promise_makeDir',
					aReason: aReason,
					curPath: pathExistsForCertain
				};
				console.error('Rejected - ' + rejObj.promiseName + ' - ', rejObj);
				deferred_makeDir_Bug934283.reject(rejObj);
			}
		).catch(
			function(aCaught) {
				console.error('Caught - promise_makeDir - ', aCaught);
				throw aCaught;
			}
		);
	};
	makeDirRecurse();

	return promise_makeDir_Bug934283;
}