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

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.

Agreed. It's a different thing, though.

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.

Thats all already covered in #5951

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 can work on windows, actually. It needs to be fixed though.

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

Remove this.

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.

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

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

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.

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

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

TODO: this is done

expand_path is utterly broken beyond repair on windows.

would be solved by #5550

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

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

Is this const used anywhere?

The spec suite.

So :nodoc:?

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

Then it should be documented with a proper description.

Oh yeah, good point. Thanks.

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

These changes are made because expect_raises is stubbed out on windows (in a future PR) and returns nil.

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)

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.

Why three levels of ..?

Why not? Thats closest to how it originally was.

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.

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.

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

Why these changes?

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.

Maybe better to_native_path?

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.

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?

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.

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

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.

Why remove drive from path?

Because FileUtils.mkdir_p can't handle windows paths.

How about adding a TODO?

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

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.

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

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.

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

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.

Wrap the names in backticks?

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

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.

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.

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.

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

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.

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

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