-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement File and Dir for win32 #5623
Conversation
CI failed because of formatting... I'll fix this when #5584 is merged. |
spec/std/dir_spec.cr
Outdated
Dir.mkdir(path, 0o700) | ||
Dir.empty?(path).should be_true | ||
path = "#{__DIR__}/data/crystal_empty_test/" | ||
begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would perhaps benefit from a helper implementing the creation and ensured removal of a directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What needs to happen is we need to provide appropriate tools to cross-compile the spec suite (i.e. never depend on the current directory and be able to tell the spec suite the location of the data and temporary dirs to use) with global helper methods. I think that's neccesary for the whole spec suite which works with files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. It's a different thing, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we must change this, a helper to create/drop a tmp folder wouldn't hurt, and avoid specs to duplicate a lot of behavior.
def in_tmpdir
tmpdir = ENV["TMPDIR"]? || ENV["TEMP"] || ENV["TMP"] || "/tmp"
path = File.join(tmpdir, "crystal-stdspec-#{Random.hex}")
begin
Dir.mkdir_p(path)
yield path
ensure
FileUtils.rm_rf(path) if Dir.exists?(path)
end
end
in_tmpdir do |path|
subpath = File.join(path, "test-mkdir")
Dir.mkdir(subpath, 0o700)
Dir.empty?(subpath).should be_true
end
That doesn't solve the need for a $DATADIR
environment variable, but sources and destinations are different matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats all already covered in #5951
src/file.cr
Outdated
{% unless flag?(:win32) %} | ||
# Yields an `IO` to read a section inside this file. | ||
# Multiple sections can be read concurrently. | ||
def read_at(offset, bytesize, &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need a way to designate methods that are not available on all platforms in the API docs. This could be a plain NOTE
badge for now but it woudl probably be better having a dedicated means for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can work on windows, actually. It needs to be fixed though.
src/crystal/system/win32/time.cr
Outdated
@@ -26,6 +30,11 @@ module Crystal::System::Time | |||
{seconds, nanoseconds} | |||
end | |||
|
|||
def self.from_filetime(filetime) : ::Time | |||
seconds, nanoseconds = filetime_to_seconds_and_nanoseconds(filetime) | |||
::Time.new(seconds: seconds, nanoseconds: nanoseconds, location: ::Time::Location::UTC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
::Time.utc
. This time for real ;)
Now that #5584 got merged, this needs a rebase. |
aecc941
to
ca99542
Compare
This is rebased, but needs a few extra features like |
end | ||
|
||
def initialize(@file_type : LibC::DWORD) | ||
# @file_attributes = LibC::BY_HANDLE_FILE_INFORMATION.new(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this.
The most useful ideas would be on how to clean up the spec suite to work better on windows. @straight-shoota's tempfile and data directory enhancements would probably be helpful too. |
spec/std/file_spec.cr
Outdated
if idx == 0 | ||
line.should eq("Hello World") | ||
{% unless flag?(:win32) %} | ||
# TODO: these are broken on win32 because they leak file descriptors which break later tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge problem: a file can only be opened once on windows. This test tests the each_line
iterator, which leaks a filedescriptor and therefore breaks all the tests after it.
I think the best way to solve this is to remove the File.each_line
iterator (keep the block version) and force people to use
File.open(...) do |file|
file.each_line. ...
end
to make leaking the file descriptor harder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to raise on windows inside File.each_line
iterator, leaving this functionality on other OSes intact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because leaking file descriptors is still a problem on unix, it's just far more of a visible problem on windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another suitable alternative to File.each_line(path)
is File.read_lines(path).each
which reads the lines into memory and iterates over the array.
spec/std/file_spec.cr
Outdated
@@ -126,7 +130,9 @@ describe "File" do | |||
ex = expect_raises(Errno, /Error determining size/) do | |||
File.empty?("#{__FILE__}/") | |||
end | |||
ex.errno.should eq(Errno::ENOTDIR) | |||
{% unless flag?(:win32) %} | |||
ex.errno.should eq(Errno::ENOTDIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are because expect_raises
is stubbed out on win32
, so the return type changes.
spec/std/file_spec.cr
Outdated
@@ -329,7 +337,11 @@ describe "File" do | |||
begin | |||
File.write(path, "") | |||
File.chmod(path, 0o775) | |||
File.info(path).permissions.should eq(File::Permissions.new(0o775)) | |||
{% if flag?(:win32) %} | |||
File.info(path).permissions.should eq(File::Permissions.new(0o666)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows doesn't have the full permissions model, so i'm not sure how to clean up these specs.
spec/std/file_spec.cr
Outdated
r.info.type.should eq(File::Type::Pipe) | ||
w.info.type.should eq(File::Type::Pipe) | ||
{% unless flag?(:win32) %} | ||
# TODO: Implement IO.pipe for windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: this is done
spec/std/file_spec.cr
Outdated
File.expand_path("a../b").should eq(File.join([base, "a../b"])) | ||
end | ||
{% unless flag?(:win32) %} | ||
describe "expand_path" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expand_path
is utterly broken beyond repair on windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be solved by #5550
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. That's why I'm pushing for it to be merged.
spec/std/file_spec.cr
Outdated
File.real_path("/usr/share").should eq("/usr/share") | ||
File.real_path("/usr/share/..").should eq("/usr") | ||
{% if flag?(:win32) %} | ||
File.real_path("/usr/share").should eq("C:\\usr\\share") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on Dir.current
being on C:\\
...
Another painful thing is that porting the spec runner to win32 depends on |
ca99542
to
5a91867
Compare
Updated the PR, still WIP because it's failing a few specs, but ready for review. |
All specs pass now. |
src/file.cr
Outdated
|
||
# :nodoc: | ||
DEFAULT_CREATE_PERMISSIONS = File::Permissions.new(0o644) | ||
|
||
{% if flag?(:win32) %} | ||
DEVNULL = "NUL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this const used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So :nodoc:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...no, why would an application not need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it should be documented with a proper description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, good point. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to review correctly. Too many changes in specs, and all the stubs force an indentation that makes things even harder to follow —I wish the formatter accepted to indent macro bodies or not, that would really help here.
Some notes:
- I don't understand the file separator being
/
: did microsoft change to support either (which version)? or do we manipulate to change/
into\
? - I don't understand the root being
/
: isn't it supposed to beC:\
(or any letter)? - Instead of changing
/tmp
to__DIR__
, could we rely on the$TMPDIR
or$TEMP
environment variables? maybe usingTMPDIR = "#{ENV["TMPDIR"]}/crystal-test-#{Process.pid}"
or something like that. - I believe
__DIR__
should include the leadingC:
It's inconsistent otherwise, because sometimes we get a leadingC:
but sometimes we don't :/
Also, green specs should mean that everything is implemented (done!). A spec that should eventually pass but doesn't for some reason must fail. Let it fail, or stub the spec body if it can't compile, but make sure to flunk (not skip).
spec/std/dir_spec.cr
Outdated
@@ -8,6 +9,10 @@ private def it_raises_on_null_byte(operation, &block) | |||
end | |||
end | |||
|
|||
private def from_win(files) | |||
files.map { |f| f.lchop("C:\\") } | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should return files
unless the :win32
flag is set.
@ysbaddaden Try adding |
Most of the details for Windows paths are actually covered in #5635, including a large spec suite. |
I think i'll make them pending. And yeah, as @straight-shoota said, windows has implemented fairly "unix-compatible" paths since aparrently MS-DOS 2 (just not at the commandline, since |
Sigh. Windows is even more horrible and confusing than I could believe. |
@ysbaddaden just wait until you see how many pages of code it takes to dereference a symlink. |
spec/std/dir_spec.cr
Outdated
@@ -47,7 +47,9 @@ describe "Dir" do | |||
ex = expect_raises(Errno, /Error determining size of/) do | |||
Dir.empty?(datapath("dir", "f1.txt", "/")) | |||
end | |||
ex.errno.should eq(Errno::ENOTDIR) | |||
{% unless flag?(:win32) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are made because expect_raises
is stubbed out on windows (in a future PR) and returns nil
.
describe "File" do | ||
it "gets path" do | ||
path = datapath("test_file.txt") | ||
file = File.new path | ||
file.path.should eq(path) | ||
File.open(path) do |file| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these changes from File.new
to File.open
are required because files are locked by default on windows, so file descriptor leaks are far more serious (and they're mostly required to make specs pass)
spec/std/file_spec.cr
Outdated
|
||
with_tempfile("test_file_symlink.txt") do |symlink| | ||
File.symlink(file, symlink) | ||
File.symlink(to_windows_path(file), symlink) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything handles unix-style paths on windows apart from the target of File.symlink
. Because Path
PR isn't merged yet, path handling remains broken in windows, so hacks like this are required to get specs to run.
spec/std/dir_spec.cr
Outdated
File.join(base_path, "subdir2", ""), | ||
].sort | ||
Dir.cd(datapath) do | ||
base_path = "../../../spec/std/data/dir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why three levels of ..
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? Thats closest to how it originally was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example is about having a path start with ..
. It doesn't need multiple. Besides, this path makes assumptions about the location of datapath
which is not strictly wrong, but can be avoided.
My suggestion:
Dir.cd(datapath("dir")) do
base_path = "../dir"
# ...
This is much shorter and clearly focuses the purpose of this spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if you're fine with it, IIRC you wrote this spec in the first place.
I saw the original spec went through a few hoops to get more than one ../
in the path, so I did too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please. The original spec was just Dir["../crystal/spec/std/data/dir/*"]
. Changing the current dir to datapath("dir")
simplifies this a lot.
spec/std/file_spec.cr
Outdated
File.expand_path("~/").should eq(home) | ||
File.expand_path("~/..badfilename").should eq(File.join(home, "..badfilename")) | ||
File.expand_path("..").should eq("/#{base.split('/')[0...-1].join('/')}".gsub(%r{\A//}, "/")) | ||
File.expand_path("..").should eq("/#{base.split('/')[0...-1].join('/')}".gsub(/\A\/\//, "/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These used to be inside a {% if flag...
, and then I found out about #6180.
Not neccesary any more because they're not in a macro.
spec/std/file_spec.cr
Outdated
{% end %} | ||
end | ||
|
||
private def to_windows_path(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to_native_path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also temporary, should be fixed up and removed in whatever PR we add proper windows path support, hopefully #5550. So I don't think it matters.
I'll probably add a bunch of TODO
comments, actually.
@@ -4,3 +4,13 @@ require "../support/tempfile" | |||
def datapath(*components) | |||
File.join("spec", "std", "data", *components) | |||
end | |||
|
|||
{% if flag?(:win32) %} | |||
def pending_win32(description = "assert", file = __FILE__, line = __LINE__, end_line = __END_LINE__, &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty straightforward implementation, but having all those pending_win32
in the specs looks a bit strange. The examples are foremost actual specs and should be called it
. Maybe it "foobar", win32: :pending
would be a better alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's overthinking it. Unless you want to make a pull request to Spec
itself I see no reason to make this look super fancy when all these pendings will be removed sooner or later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's probably not worth it changing it
. But maybe there is a better name for this method...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, it's clear enough and easy enough to grep for the method definition. We're hardly writing public API here, code quality and naming of methods in spec files is really not worth bikeshedding about since it's so easy to change.
@@ -1,7 +1,11 @@ | |||
require "tempfile" | |||
require "file_utils" | |||
|
|||
SPEC_TEMPFILE_PATH = File.join(Tempfile.dirname, "cr-spec-#{Random.new.hex(4)}") | |||
{% if flag?(:win32) %} | |||
SPEC_TEMPFILE_PATH = File.join(Tempfile.dirname, "cr-spec-#{Random.new.hex(4)}").gsub("C:\\", '/').gsub('\\', '/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove drive from path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because FileUtils.mkdir_p
can't handle windows paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would ne nice to have something like .gsub(/\A[a-zA-Z]:\\\\/)
which works with a temp path on other drives than C:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO is to merge #5550. And honestly, I'm just hacking the specs around until they work here, then cleaning it up enough to be good to merge. At the end of it all we can just grep -R 'flag\?(:win32' src
and find all this junk once we merge proper path handling support.
We could make the spec code super solid, or we could not faff around and merge it. Supporting enough to work on a test VM is fine for now. Once the windows port is solid enough that the specs don't have to be hacked around, then we can remove the hacks. Until then, the hack needs to be somwhere, and a shitty hack will remind us about it's existance way earlier if we forget it.
@file_attributes.nFileIndexLow == other.file_attributes.nFileIndexLow | ||
end | ||
|
||
private def to_windows_path(path : String) : LibC::LPWSTR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's trivial, but could maybe reuse Crystal::System::Dir.to_windows_path
, optionally moved to a different location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little bit of copying never did no harm, at least for a tiny method like this. It'd cost more time and characters and thought to make this DRY rather than just copying it. The method contains no logic anyway, it's just a shortcut for a chain of methods.
16f0b00
to
64f25cb
Compare
Ready for another review |
src/file.cr
Outdated
@@ -2,22 +2,31 @@ require "crystal/system/file" | |||
|
|||
class File < IO::FileDescriptor | |||
# The file/directory separator character. `'/'` in Unix, `'\\'` in Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this and the other comment be changed since /
is a legal file separator in Unix and Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these should be changed to just say they're always /
. This will be changed anyway once we have Path
.
I'll wait until more comments arrive to fix this.
src/file.cr
Outdated
@@ -10,6 +10,17 @@ class File < IO::FileDescriptor | |||
# :nodoc: | |||
DEFAULT_CREATE_PERMISSIONS = File::Permissions.new(0o644) | |||
|
|||
# The name of the null device on the host platform. /dev/null on UNIX and NUL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap the names in backticks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supporting exceptions and ENV should come before this PR IMO.
@@ -27,11 +27,23 @@ private def it_raises_on_null_byte(operation, &block) | |||
end | |||
end | |||
|
|||
private def normalize_permissions(permissions, *, directory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this helper is needed? Is it a temporal thing or an issue with the abstraction across platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows doesn't have the concept of unix permissions - there's a single "readonly" bit accessible at this API level (you can go and do the full ACL permissions but... no thanks)
So you have to work around that in the specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't expect to do the full ACL.
But either the feature is not enabled/tested on windows, or
there is some translation in the implementation to map (at some extent) the result from windows to the stdlib permission value and have a unified API in both platform.
Neither of these options requires a normalize_permission
in the specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It maps incoming permissions just fine, the problem is that it's a lossy transform, so that when the spec suite comes back and queries the permissions it just set on file xyz it gets different permissions to what it just set back. This helper is manually performing the same lossy transformation in crystal code that happens in windows' libc
to make sure the permissions like up.
spec/std/file_spec.cr
Outdated
@@ -82,7 +94,8 @@ describe "File" do | |||
idx.should eq(20) | |||
end | |||
|
|||
it "reads lines from file with each as iterator" do | |||
# TODO: following two specs are broken on win32 because they leak file descriptors which break later tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do they leak? is it because the ensure are not handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, ensure works fine you just can't assume that the GC runs between these specs and others. Relying on File#ensure
is a hack. Regardless, the purpose of #6301 was to remove these methods because they always leak file references. And that's merged so this is all moot since these specs are gone.
elsif small_buf && len > 0 | ||
next len | ||
else | ||
raise WinError.new("Error resolving real path of #{path.inspect}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for later, but I think there should be a well defined hierarchy of exception across platforms. Otherwise the API is not portable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a new approach following #5003 would be great!
I think that's perfectionism. This works now and the spec diff is minimal. Bikeshedding about the order is moot when the end result will be the same. The reason that I'm doing |
@bcardiff I thought the same, exceptions should come first, but I think merging this pull request is okay. It's isolated, as it implements target specific stuff, it doesn't break anything and is one step forward supporting windows. Let's merge, once ready. |
I intent to make a suggestion. Without exception it's hard to trust the result of the specs or use the result of the failure to narrow the what went wrong. It could end in more time been consumed on your side. But I am not against merging because of the lack of it. To be clear, besides waiting for the related PR and rebase, the only outstanding thing on my side is the comment regarding |
64f25cb
to
be26b51
Compare
Final few reviewes fixed up and rebased. Final review for merge? |
Aren't we waiting on #5635 before merging this PR? |
No, its orthogonal. |
Then LGTM |
Finally 🎉 |
This is fairly WIP, since it depends on #5584 which isn't finalized. It works though.
The main goal is to continue to reduce the amount of stubbing out in
file_spec
anddir_spec
.