fix Issue 15771 - FileLogger should create the output directory if it… by burner · Pull Request #5370 · dlang/phobos

burner · 2017-05-07T09:38:02Z

… does not exist

dlang-bot · 2017-05-07T09:38:03Z

Thanks for your pull request, @burner!

Bugzilla references

Auto-close	Bugzilla	Description
✓	15771	FileLogger should create the output directory if it does not exist

wilzbach

I found the documentation quite hard to read and I added a couple of small nits.

wilzbach · 2017-05-08T19:08:16Z

std/experimental/logger/filelogger.d

+        if (createFileNameFolder && !exists(d))
+        {
+            () @trusted { mkdirRecurse(d); }();
+            assert(exists(d));


assert should always come with a helpful error message.

~~Hm... mkdirRecurse is already telling you whether it made the directory or not. Why not just assert that?~~ Edit: no it's not. I misread the code. But it is already checking that the directory exists, see https://github.com/dlang/phobos/blob/master/std/file.d#L2308, so we can probably remove the assert altogether.

~~As far as a helpful message, I don't know the requirements, but isn't printing the line number and file (as asserts always do) helpful enough?~~ No need if we remove the assert.

Asserting on I/O errors is almost always a mistake. If mkdirRecurse's contract is that on its successful return (and barring any race conditions you can't avoid anyway) the directory exists, then the program should assume the directory exists - thus the assert is redundant.

wilzbach · 2017-05-08T19:10:12Z

std/experimental/logger/filelogger.d

+        auto d = dirName(this.filename);
+        if (createFileNameFolder && !exists(d))
+        {
+            () @trusted { mkdirRecurse(d); }();


What's the underlying assumption that mkdirRecurse will be @safe.
Imho we should eat our dogfood and make Phobos usable with @safe and not create more holes.

none, but the Issue can't be fixed without mkdirRecurse. It is not safe as it uses a C function again.

but we can't simply add @trusted just to make functions @safe ...
CC @schveiguy

I'm open to suggestions

Looking at mkdirRecurse, I think the trusted escape is OK. However, the best solution is to actually make mkdirRecurse @safe, and put the trusted escape on the C call inside there. If you want to do that, it would benefit others as well.

I don't see why mkdirRecurse shouldn't @safe - mkdir is already @safe.

However, the best solution is to actually make mkdirRecurse @safe

Shouldn't be too difficult: #5386

ping @burner, mkdirRecurse is now @safe.

cool, I'll change this PR

wilzbach · 2017-05-08T19:13:34Z

std/experimental/logger/filelogger.d

      file can not be opened for writting an exception will be thrown.
      lv = The $(D LogLevel) for the $(D FileLogger). By default the
      $(D LogLevel) for $(D FileLogger) is $(D LogLevel.all).
+      createFileNameFolder = if true and fn contains a folder name, this


Nits:

How about using a Flag instead of a boolean?

How about naming it to sth. similar to make -p
makeParentDirectories = If set to true`, parent directories will be created as needed

Definitely use Flag. We've essentially decided that all new Phobos APIs that take bool parameters to take Flags instead.

wilzbach · 2017-05-08T19:21:08Z

std/experimental/logger/filelogger.d

+        this(fn, lv, false);
+    }
+
+    this(in string fn, const LogLevel lv, bool createFileNameFolder) @safe


Btw CircleCi is failing due to

std/experimental/logger/filelogger.d(34): Warning: Ddoc: function declaration has no parameter 'createFileNameFolder' std/experimental/logger/filelogger.d(34): Warning: Ddoc: parameter count mismatch make: *** [style] Error 1

(you will need /// ditto to fix this)

CyberShadow · 2017-05-12T13:10:05Z

std/experimental/logger/filelogger.d

+
        this.filename = fn;
+        auto d = dirName(this.filename);
+        if (createFileNameFolder && !exists(d))


mkdirRecurse will already check for existence. The exists check is redundant, remove it.

CyberShadow · 2017-05-12T13:10:42Z

std/experimental/logger/filelogger.d

        super(lv);
+
        this.filename = fn;
+        auto d = dirName(this.filename);


Move this into the if (createFileNameFolder) block to avoid a calculation that's unnecessary for the default case.

burner · 2017-05-17T20:15:45Z

all fixed up I hope

joakim-noah · 2017-06-23T04:28:20Z

std/experimental/logger/filelogger.d

+{
+    import std.file : exists, rmdirRecurse;
+
+    string logpath = "super/long/path/";


Please prepend with std.file.deleteme, don't just create test files in the local directory.

FYI: deleteme just returns a string sth. like /tmp/deleteme.dmd.unittest.pid1234 - it can be used to construct temporary folders as well.

wilzbach

@burner - how about a final rebase to get this in?

wilzbach · 2017-07-02T19:32:49Z

std/experimental/logger/filelogger.d

+            auto d = dirName(this.filename);
+            mkdirRecurse(d);
+            assert(exists(d), "The folder the FileLogger should have been"
+                ~ " created in '" ~ d ~ "' could not be created.");


You can use std.conv.text to make this a bit more efficient and legible (text uses an appender)

wilzbach · 2017-07-02T19:37:19Z

std/experimental/logger/filelogger.d

+{
+    import std.file : exists, rmdirRecurse;
+
+    string logpath = "super/long/path/";


FYI: deleteme just returns a string sth. like /tmp/deleteme.dmd.unittest.pid1234 - it can be used to construct temporary folders as well.

wilzbach · 2017-07-02T19:37:55Z

std/experimental/logger/filelogger.d

+
+    fl.file.close();
+    () @trusted { rmdirRecurse(logpath); }();
+    assert(!exists(logpath));


this seems unnecessary - you don't need to test rmdirRecurse

wilzbach · 2017-07-02T19:39:44Z

std/experimental/logger/filelogger.d

+    assert(exists(logpath));
+
+    fl.file.close();
+    () @trusted { rmdirRecurse(logpath); }();


How about scope(exit) in the beginning, s.t. the temporary folder is removed even in case of failures?

JackStouffer · 2017-07-11T13:41:14Z

std/experimental/logger/filelogger.d

        super(lv);
        this.filename = fn;
+
+        if (createFileNameFolder == CreateFolder.yes)


if (createFileNameFolder)

hm, this feels like it would defeats the purpose of the Flag type.

JackStouffer · 2017-07-11T13:41:43Z

Ping @burner

burner · 2017-07-12T10:37:50Z

@wilzbach I can't use deleteme as need a bunch of subdirs to check if this patch actually fixes the problem.

… does not exist

wilzbach · 2017-07-12T11:11:13Z

@wilzbach I can't use deleteme as need a bunch of subdirs to check if this patch actually fixes the problem.

As mentioned before: "FYI: deleteme just returns a string sth. like /tmp/deleteme.dmd.unittest.pid1234 - it can be used to construct temporary folders as well."

See also: https://github.com/dlang/phobos/pull/5594/files

wilzbach · 2017-07-12T11:14:08Z

@burner @RazvanN7 - I saw that you both rebased your PRs within the last hour. I don't mind which PR you finalize, but I think you both have permissions to push to each other's PR - use them (and close the other)!

RazvanN7 · 2017-07-12T11:22:33Z

@burner My PR uses deleteme and addresses the rest of the comments, but I copy-pasted your implementation. I'm down with whatever option you prefer, but I think it would be less pain for you to just close this one as mine is already in a mergeable condition

dlang-bot added the Severity:Bug Fix label May 7, 2017

LightBender approved these changes May 7, 2017

View reviewed changes

wilzbach reviewed May 8, 2017

View reviewed changes

CyberShadow reviewed May 12, 2017

View reviewed changes

burner force-pushed the filelogger_mkdir branch from add130e to f9c5f5c Compare May 17, 2017 19:07

burner force-pushed the filelogger_mkdir branch from f9c5f5c to 1986714 Compare June 1, 2017 12:12

joakim-noah reviewed Jun 23, 2017

View reviewed changes

wilzbach reviewed Jul 2, 2017

View reviewed changes

wilzbach mentioned this pull request Jul 11, 2017

Fix Issue 15771 - FileLogger should create the output directory if it does not exist #5594

Merged

JackStouffer reviewed Jul 11, 2017

View reviewed changes

fix Issue 15771 - FileLogger should create the output directory if it…

6bfd481

… does not exist

burner force-pushed the filelogger_mkdir branch from 1986714 to 6bfd481 Compare July 12, 2017 10:38

dlang-bot added the Severity:Enhancement label Jul 12, 2017

burner closed this Jul 12, 2017

Uh oh!

Conversation

burner commented May 7, 2017

Uh oh!

dlang-bot commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schveiguy May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burner commented May 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Jul 11, 2017

Uh oh!

burner commented Jul 12, 2017

Uh oh!

wilzbach commented Jul 12, 2017

Uh oh!

wilzbach commented Jul 12, 2017

Uh oh!

RazvanN7 commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

dlang-bot commented May 7, 2017 •

edited

Loading

schveiguy May 12, 2017 •

edited

Loading

RazvanN7 commented Jul 12, 2017 •

edited

Loading