Fix Issue 16487 - Add function to obtain the available disk space by jercaianu · Pull Request #5776 · dlang/phobos

jercaianu · 2017-10-10T15:44:44Z

removed redundant line

dlang-bot · 2017-10-10T15:44:47Z

Thanks for your pull request and interest in making D better, @jercaianu! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	16487	enhancement	Add function to obtain the available disk space

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#5776"

jercaianu · 2017-10-10T18:08:02Z

Thanks for the comments, I'll shortly look into them as soon as I figure out what's going on with the FreeBSD failure.

Also, since we're here, do you think it would be a good idea to add functionality for getFreeDiskSpace or getDiskSpaceCapacity (similar to boost::filesystem) ?

Thanks,
Alex

ghost

looks good so far
just a little change related to version and it's okay.

jercaianu · 2017-10-10T19:44:17Z

I have figured out what's wrong with the FreeBSD test.
It seems that statvfs does not work on FreeBSD as expected, but statfs works.

The question is now, should I add statfs in the runtime, or just assume return 0 for FreeBSD?

Thanks,
Alex

wilzbach · 2017-10-10T23:24:17Z

The question is now, should I add statfs in the runtime, or just assume return 0 for FreeBSD?

Extending the bindings in druntime is almost always welcome as *BSD is vastly lacking behind in terms of maintenance manpower (iirc only one or two of the core people around here use it regularly).

n8sh · 2018-03-27T22:33:05Z

The question is now, should I add statfs in the runtime, or just assume return 0 for FreeBSD?

You throw an exception on failure, so isn't the correct behavior to throw an exception?

quickfur · 2018-03-27T22:43:44Z

Tagging @andralex since this adds a new public symbol.

quickfur · 2018-03-27T22:44:12Z

ping @jercaianu This has stalled for far too long. Any chance we can get things moving again?

jercaianu · 2018-03-28T18:11:31Z

@quickfur
Hi, the reason I can't move forward with this, is because I need to add statvfs on FreeBSD [1].
However, this in turn, is blocked by this one [2].

If statfs would be added, then I can easily finish this.

[1] - dlang/druntime#1937
[2] - https://issues.dlang.org/show_bug.cgi?id=17596#c8

wilzbach · 2018-03-30T22:21:57Z

+            return GetDiskFreeSpaceExW(path.tempCStringW(), &freeBytesAvailable, null, null);
+        } ();
+        enforce(err != 0,
+            new FileException(text("Cannot get available disk space: ", GetLastError())));


Have a look at the default constructor of FileException:

version(Windows) this(in char[] name, uint errno = .GetLastError(), string file = __FILE__, size_t line = __LINE__) @safe {

tl;dr: use cenforce

wilzbach · 2018-03-30T22:25:20Z

As this is a new function, it might be worthwhile to check this PR.
The idea is to mark it as unstable (in some way) for a few releases.

wilzbach · 2018-03-30T22:28:41Z

+    assert(space > 0);
+
+    assertThrown!FileException(getAvailableDiskSpace("ThisFileDoesNotExist123123"));
+}


I think we can do more tests here. Simple create a directory with deleteme and write files to it and check the resulting file size (if this varies on Windows, version(Posix) it).

Example:

https://dlang.org/phobos-prerelease/std_file.html#mkdir

That'd be a bit excessive - we'd be testing the OS primitives.

Based on what you have running on your system, the size could vary a lot.
This may cause such a test to fail at times, despite everything running correctly.

You should test that the function works for null and "" (empty strings) input. See e.g. https://issues.dlang.org/show_bug.cgi?id=14980

jercaianu · 2018-04-03T14:41:15Z

This should be alright now

I would still prefer that we either move this to std.experimental for one release or mark it as unstable in the docs somehow, s.t. we don't accidentally end up releasing something that we can't change anymore (as we used to do in the past), but I won't block this PR further.

andralex · 2018-04-18T12:16:46Z

@jercaianu what's with the linux32 failure?

andralex · 2018-04-18T12:17:01Z

@JackStouffer good to go?

n8sh · 2018-04-18T12:19:34Z

@andralex Appears unrelated.

posix.mak:381: recipe for target 'unittest/std/concurrency.run' failed
make[1]: *** [unittest/std/concurrency.run] Error 139

JackStouffer · 2018-04-18T14:15:05Z

I won’t be able to review this until late tonight. Please merge without my review if you think this is good to go.

tibi77 · 2019-03-08T13:57:00Z

Why is this stil open?

thewilsonator · 2019-03-08T14:02:31Z

Because it fails on FreeBSD.

thewilsonator · 2019-06-19T03:46:02Z

@n8sh this probably needs a rebase to pass on circle.

n8sh · 2019-06-23T11:31:27Z

+    }
+    else version (Posix)
+    {
+        import std.string : toStringz;


Since this function's argument type was changed from string to const char[] it should probably use tempCString instead of toStringz in the Posix path. toStringz always allocates if it is given a const char[].

Tests are no longer disabled on FreeBSD.

JohanEngelen · 2019-06-30T15:18:18Z

#5776 (comment) ?

jercaianu requested a review from CyberShadow as a code owner October 10, 2017 15:44

dlang-bot added the Severity:Enhancement label Oct 10, 2017

ghost reviewed Oct 10, 2017

View reviewed changes

Comment thread std/file.d Outdated

ghost reviewed Oct 10, 2017

View reviewed changes

Comment thread std/file.d Outdated

ghost reviewed Oct 10, 2017

View reviewed changes

Comment thread std/file.d Outdated

ghost reviewed Oct 10, 2017

View reviewed changes

Comment thread std/file.d Outdated

ghost reviewed Oct 10, 2017

View reviewed changes

Comment thread std/file.d

nemanja-boric-sociomantic reviewed Oct 10, 2017

View reviewed changes

Comment thread std/file.d Outdated

wilzbach reviewed Oct 10, 2017

View reviewed changes

Comment thread std/file.d

joakim-noah reviewed Oct 10, 2017

View reviewed changes

Comment thread std/file.d Outdated

ghost suggested changes Oct 10, 2017

View reviewed changes

jercaianu mentioned this pull request Oct 11, 2017

Added statfs for FreeBSD dlang/druntime#1937

Merged

JackStouffer added the Merge:stalled label Nov 21, 2017

dlang-bot added Review:Needs Work Merge:Needs Rebase and removed Merge:stalled Merge:Needs Rebase labels Dec 29, 2017

dlang-bot added the Merge:stalled label Jan 9, 2018

quickfur reviewed Mar 27, 2018

View reviewed changes

Comment thread std/file.d Outdated

quickfur added the @andralex label Mar 27, 2018

jercaianu force-pushed the disk branch from c350e8d to 8b0ee32 Compare March 30, 2018 22:23

wilzbach reviewed Mar 30, 2018

View reviewed changes

jercaianu force-pushed the disk branch from 8b0ee32 to f31a1b4 Compare March 30, 2018 22:25

wilzbach reviewed Mar 30, 2018

View reviewed changes

jercaianu force-pushed the disk branch from f31a1b4 to 0bffa0b Compare April 3, 2018 12:10

jercaianu force-pushed the disk branch from 9412f64 to df9fe8a Compare April 4, 2018 11:54

wilzbach force-pushed the disk branch from df9fe8a to 8f7371b Compare April 6, 2018 14:34

wilzbach removed the @andralex label Apr 6, 2018

andralex approved these changes Apr 18, 2018

View reviewed changes

n8sh force-pushed the disk branch from 7fbd999 to 78babae Compare June 18, 2019 23:40

jercaianu and others added 3 commits June 23, 2019 05:00

Fix Issue 16487 - Add function to obtain the available disk space

d38cbf9

Changed getAvailableDiskSpace signature

d0f1960

Fix FreeBSD deprecated import

8082e7d

n8sh force-pushed the disk branch from a10c9d7 to 8082e7d Compare June 23, 2019 09:01

n8sh reviewed Jun 23, 2019

View reviewed changes

getAvailableDiskSpace: toStringz -> tempCString

425eeb3

thewilsonator added the Merge:auto-merge-squash label Jun 28, 2019

dlang-bot merged commit 6010a7c into dlang:master Jun 28, 2019

Uh oh!

Conversation

jercaianu commented Oct 10, 2017

Uh oh!

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

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jercaianu commented Oct 10, 2017

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jercaianu commented Oct 10, 2017

Uh oh!

wilzbach commented Oct 10, 2017

Uh oh!

n8sh commented Mar 27, 2018

Uh oh!

Uh oh!

quickfur commented Mar 27, 2018

Uh oh!

quickfur commented Mar 27, 2018

Uh oh!

jercaianu commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilzbach Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilzbach commented Mar 30, 2018

Uh oh!

wilzbach Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

andralex Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

jercaianu Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

JohanEngelen Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

jercaianu commented Apr 3, 2018

Uh oh!

andralex commented Apr 18, 2018

Uh oh!

andralex commented Apr 18, 2018

Uh oh!

n8sh commented Apr 18, 2018

Uh oh!

JackStouffer commented Apr 18, 2018

Uh oh!

tibi77 commented Mar 8, 2019

Uh oh!

thewilsonator commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thewilsonator commented Jun 19, 2019

Uh oh!

n8sh Jun 23, 2019

Choose a reason for hiding this comment

Uh oh!

JohanEngelen commented Jun 30, 2019

dlang-bot commented Oct 10, 2017 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

jercaianu commented Mar 28, 2018 •

edited

Loading

thewilsonator commented Mar 8, 2019 •

edited

Loading