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

Merged
merged 3 commits into from Jul 5, 2018

Conversation

@RX14
Member

RX14 commented Jan 21, 2018

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 and dir_spec.

@RX14

This comment has been minimized.

Member

RX14 commented Jan 21, 2018

CI failed because of formatting... I'll fix this when #5584 is merged.

Dir.mkdir(path, 0o700)
Dir.empty?(path).should be_true
path = "#{__DIR__}/data/crystal_empty_test/"
begin

This comment has been minimized.

@straight-shoota

straight-shoota Jan 22, 2018

Contributor

This would perhaps benefit from a helper implementing the creation and ensured removal of a directory.

This comment has been minimized.

@RX14

RX14 Jan 22, 2018

Member

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.

This comment has been minimized.

@straight-shoota

straight-shoota Jan 22, 2018

Contributor

Agreed. It's a different thing, though.

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 17, 2018

Member

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.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 17, 2018

Contributor

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)

This comment has been minimized.

@straight-shoota

straight-shoota Jan 22, 2018

Contributor

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.

This comment has been minimized.

@RX14

RX14 Jan 22, 2018

Member

This can work on windows, actually. It needs to be fixed though.

@@ -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)

This comment has been minimized.

@straight-shoota

straight-shoota Jan 22, 2018

Contributor

::Time.utc. This time for real ;)

@RX14 RX14 referenced this pull request Apr 12, 2018

Open

Implement Path type #5635

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Apr 15, 2018

Now that #5584 got merged, this needs a rebase.

@RX14 RX14 force-pushed the RX14:feature/windows-file-dir branch from aecc941 to ca99542 Apr 15, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Apr 15, 2018

This is rebased, but needs a few extra features like pread, and to fix @straight-shoota's comments. Feel free to review the rest of the PR though.

@RX14 RX14 added the pr:wip label Apr 15, 2018

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)

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

Remove this.

@RX14

This comment has been minimized.

Member

RX14 commented Apr 15, 2018

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.

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

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

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.

This comment has been minimized.

@Sija

Sija Apr 15, 2018

Contributor

Wouldn't be better to raise on windows inside File.each_line iterator, leaving this functionality on other OSes intact?

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

No, because leaking file descriptors is still a problem on unix, it's just far more of a visible problem on windows.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 15, 2018

Contributor

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.

@@ -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)

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

These are because expect_raises is stubbed out on win32, so the return type changes.

@@ -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))

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

Windows doesn't have the full permissions model, so i'm not sure how to clean up these specs.

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

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

TODO: this is done

File.expand_path("a../b").should eq(File.join([base, "a../b"]))
end
{% unless flag?(:win32) %}
describe "expand_path" do

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

expand_path is utterly broken beyond repair on windows.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 15, 2018

Contributor

would be solved by #5550

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

Indeed. That's why I'm pushing for it to be merged.

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")

This comment has been minimized.

@RX14

RX14 Apr 15, 2018

Member

This depends on Dir.current being on C:\\...

@RX14

This comment has been minimized.

Member

RX14 commented Apr 15, 2018

Another painful thing is that porting the spec runner to win32 depends on File and Dir, and obviously the File and Dir specs in this PR depend on the spec runner. So until we get spec merged, there's specs in the stdlib we can't run.

@RX14 RX14 force-pushed the RX14:feature/windows-file-dir branch from ca99542 to 5a91867 Apr 15, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Apr 15, 2018

Updated the PR, still WIP because it's failing a few specs, but ready for review.

@RX14

This comment has been minimized.

Member

RX14 commented Apr 15, 2018

All specs pass now.

@RX14 RX14 removed the pr:wip label Apr 15, 2018

src/file.cr Outdated
# :nodoc:
DEFAULT_CREATE_PERMISSIONS = File::Permissions.new(0o644)
{% if flag?(:win32) %}
DEVNULL = "NUL"

This comment has been minimized.

@straight-shoota

straight-shoota Apr 17, 2018

Contributor

Is this const used anywhere?

This comment has been minimized.

@RX14

RX14 Apr 17, 2018

Member

The spec suite.

This comment has been minimized.

@straight-shoota

straight-shoota Apr 17, 2018

Contributor

So :nodoc:?

This comment has been minimized.

@RX14

RX14 Apr 17, 2018

Member

...no, why would an application not need this?

This comment has been minimized.

@straight-shoota

straight-shoota Apr 17, 2018

Contributor

Then it should be documented with a proper description.

This comment has been minimized.

@RX14

RX14 Apr 17, 2018

Member

Oh yeah, good point. Thanks.

@ysbaddaden

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 be C:\ (or any letter)?
  • Instead of changing /tmp to __DIR__, could we rely on the $TMPDIR or $TEMP environment variables? maybe using TMPDIR = "#{ENV["TMPDIR"]}/crystal-test-#{Process.pid}" or something like that.
  • I believe __DIR__ should include the leading C: It's inconsistent otherwise, because sometimes we get a leading C: 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).

@@ -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

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 17, 2018

Member

That should return files unless the :win32 flag is set.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Apr 17, 2018

@ysbaddaden Try adding ?w=1to the compare URL, it'll hide lines from the diff with only whitespace changes (like -w). Makes reviewing much easier (although you can't directly comment in this view).

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Apr 17, 2018

@ysbaddaden

  • Forward slash / is a legal file separator on Windows, it has even supported since DOS times.
  • A path starting with / or \ on windows is rooted in the current drive. It's not an absolute path, but rooted.
  • This should work using Dir.tempdir which is implemented in this PR or even better with_tempfile from #5951.

Most of the details for Windows paths are actually covered in #5635, including a large spec suite.

@RX14

This comment has been minimized.

Member

RX14 commented Apr 17, 2018

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).

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 / is the option string). The reason i'm not even attempting to try and use true windows paths is because it would make this PR even more horifically large than it is. Before I removed the code that made the seperator \ on windows, all the path handling commands were entirely broken because they assumed that \ was the seperator on windows. It's not, both \ and / are seperators and so you got \/\/ monstrosities and all the specs were broken. So until we merge Path, and sort this out properly, the specs are essentially a hack. But the hack works pretty well if you only have one drive.

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Apr 18, 2018

Sigh. Windows is even more horrible and confusing than I could believe.

@RX14

This comment has been minimized.

Member

RX14 commented Apr 18, 2018

@ysbaddaden just wait until you see how many pages of code it takes to dereference a symlink.

File.join(base_path, "subdir2", ""),
].sort
Dir.cd(datapath) do
base_path = "../../../spec/std/data/dir"

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

Why three levels of ..?

This comment has been minimized.

@RX14

RX14 Jun 27, 2018

Member

Why not? Thats closest to how it originally was.

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

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.

This comment has been minimized.

@RX14

RX14 Jun 28, 2018

Member

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.

This comment has been minimized.

@straight-shoota

straight-shoota Jun 28, 2018

Contributor

Yes please. The original spec was just Dir["../crystal/spec/std/data/dir/*"]. Changing the current dir to datapath("dir") simplifies this a lot.

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\/\//, "/"))

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

Why these changes?

This comment has been minimized.

@RX14

RX14 Jun 27, 2018

Member

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.

{% end %}
end
private def to_windows_path(path)

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

Maybe better to_native_path?

This comment has been minimized.

@RX14

RX14 Jun 28, 2018

Member

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)

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

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?

This comment has been minimized.

@RX14

RX14 Jun 27, 2018

Member

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.

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

Yeah, that's probably not worth it changing it. But maybe there is a better name for this method...?

This comment has been minimized.

@RX14

RX14 Jun 28, 2018

Member

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('\\', '/')

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

Why remove drive from path?

This comment has been minimized.

@RX14

RX14 Jun 27, 2018

Member

Because FileUtils.mkdir_p can't handle windows paths.

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

How about adding a TODO?

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

It would ne nice to have something like .gsub(/\A[a-zA-Z]:\\\\/) which works with a temp path on other drives than C:.

This comment has been minimized.

@RX14

RX14 Jun 28, 2018

Member

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

This comment has been minimized.

@straight-shoota

straight-shoota Jun 27, 2018

Contributor

It's trivial, but could maybe reuse Crystal::System::Dir.to_windows_path, optionally moved to a different location.

This comment has been minimized.

@RX14

RX14 Jun 27, 2018

Member

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.

@RX14 RX14 force-pushed the RX14:feature/windows-file-dir branch 2 times, most recently from 16f0b00 to 64f25cb Jul 1, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Jul 2, 2018

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.

This comment has been minimized.

@r00ster91

r00ster91 Jul 2, 2018

Contributor

Shouldn't this and the other comment be changed since / is a legal file separator in Unix and Windows?

This comment has been minimized.

@RX14

RX14 Jul 2, 2018

Member

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

This comment has been minimized.

@straight-shoota

straight-shoota Jul 2, 2018

Contributor

Wrap the names in backticks?

@bcardiff

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)

This comment has been minimized.

@bcardiff

bcardiff Jul 3, 2018

Member

Why this helper is needed? Is it a temporal thing or an issue with the abstraction across platforms?

This comment has been minimized.

@RX14

RX14 Jul 3, 2018

Member

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.

This comment has been minimized.

@bcardiff

bcardiff Jul 4, 2018

Member

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.

This comment has been minimized.

@RX14

RX14 Jul 4, 2018

Member

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.

@@ -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

This comment has been minimized.

@bcardiff

bcardiff Jul 3, 2018

Member

why do they leak? is it because the ensure are not handled?

This comment has been minimized.

@RX14

RX14 Jul 3, 2018

Member

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}")

This comment has been minimized.

@bcardiff

bcardiff Jul 3, 2018

Member

Maybe for later, but I think there should be a well defined hierarchy of exception across platforms. Otherwise the API is not portable.

This comment has been minimized.

@straight-shoota

straight-shoota Jul 3, 2018

Contributor

Yes, a new approach following #5003 would be great!

@RX14

This comment has been minimized.

Member

RX14 commented Jul 3, 2018

Supporting exceptions and ENV should come before this PR IMO

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 File and Dir first is that it's the minimal requirement to support the spec suite runner. After porting the spec suite runner, it's a lot easier to branch out to other stuff.

@bararchy

This comment has been minimized.

Contributor

bararchy commented Jul 4, 2018

I'm with @RX14 especially now that @txe dropped working on exceptions, we should merge W/E is ready so we won't discourage people from working on features, the more waiting we do the more burnout people become

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Jul 4, 2018

@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.

@bcardiff

This comment has been minimized.

Member

bcardiff commented Jul 4, 2018

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 normalize_permissions.

RX14 added some commits Dec 24, 2017

@RX14 RX14 force-pushed the RX14:feature/windows-file-dir branch from 64f25cb to be26b51 Jul 4, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Jul 4, 2018

Final few reviewes fixed up and rebased. Final review for merge?

@bcardiff

This comment has been minimized.

Member

bcardiff commented Jul 4, 2018

Aren't we waiting on #5635 before merging this PR?

@RX14

This comment has been minimized.

Member

RX14 commented Jul 4, 2018

No, its orthogonal.

@bararchy

This comment has been minimized.

Contributor

bararchy commented Jul 4, 2018

Then LGTM

@sdogruyol

This comment has been minimized.

Member

sdogruyol commented Jul 5, 2018

Finally 🎉

@sdogruyol

sdogruyol approved these changes Jul 5, 2018 edited

@sdogruyol sdogruyol merged commit b5ef578 into crystal-lang:master Jul 5, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bcardiff bcardiff added this to the Next milestone Jul 5, 2018

@RX14 RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment