Crystal: Use a global tmpdir for specs

[bew]

5810 adds a few ensure after specs that creates tempfiles but don't delete them.

To remove all of this we could also create a temp directory in spec_helper.cr using:

dirname = Tempfile.tempname "compiler_spec"
Dir.mkdir dirname
ENV["TMPDIR"] = dirname

So all subsequent Tempfile calls will create a file in our dirname, and we could just delete it after all specs passes (or not?).

Maybe if the specs doesn't passes we can leave the directory and tell the user that it's there if he wants to checkout some leftover file?

WDYT?

[straight-shoota]

I'm against leaving tempfiles in place to avoid compromising subsequent spec runs. If you need to investigate a failing spec, you should adjust the spec to temporarily include diagnostics or not delete the file.

About the centralized temp directory I'm not so sure... by itself it sounds like a nice idea, but I wouldn't want individual specs to rely on some global cleanup magic instead of doing their own housekeeping.

As an alternative, I'd suggest a spec helper with_tempfile which yields a block receiving a temp path and ensures that path is cleaned up at the end. So it would actually be like Tempfile.open for Tempfile.tempname. The path could actually be in a central spec directory according to your proposal. But every spec needs to do it's cleanup itself.

def with_tempfile(extension = nil)
  # naive implementation
  path = Tempfile.tempname(extension)
  begin
    yield path
  ensure
    File.delete(path)
  end
end

This might even be implemented as Tempfile.tempname receiving a block... it could surely prove useful outside of specs that you want to ensure a temporary file is cleaned up, but need to handle file creation yourself.

[RX14]

The current problem is that it's almost impossible to cross-compile the specs because there's no centralised temporary directory (lots of tests just dump their tempfiles in the repo and then "clean up"), AND all sorts of tests use __FILE__ and __DIR__. These two constants break cross-compilation. I suggest if we're having a custom tempdir helper we should have a custom datafiles helper too which allows you to place any supporting data files related to the specs in a configurable location.

[straight-shoota]

Data file helper sounds good, but why don't __FILE__ and __DIR__ cross-compile?

[RX14]

@straight-shoota they do, they just point to paths on the host not the target so are useless. Especially when __FILE__ is /data/programming/crystal-lang/crystal/windows/spec/std/file_spec.cr so you have to check out crystal at C:\data\programming\crystal-lang\crystal\windows to make it work again. That's just ugly.

[straight-shoota]

So the goal should be to get rid of most __FILE__ and __DIR__ usages in specs and replace them with tempfile and datafile helpers. These are actually two different issues but it makes sense to do the replacements in one go. It could still be separated commits/PRs with selective committing (git add -p). We should probably have separate PRs to implement the helpers first.

Temporary files should generally just go where $TMPDIR says (default /tmp). All temporary files should be put in that path, but could be scoped to a sub directory $TMPDIR/crystal.

In specs, you typically don't really need temporary files using Tempfile (which adds name randomization) because most file names are unique and not reused. Random characters in the file names make them harder to read and compare. A spec helper should avoid unnecessary randomization and probably just use the plain names (scoped to a sub directory $TMPDIR/crystal).

• It probably still makes sense to add Tempfile.tempfile(&block) as a general purpose utility to create and automatically delete a Tempfile after the block scope exits. But that's a separate issue.
• A spec helper with_tempfile (or perhaps a different name to avoid confusion with Tempfile?). This could either go in a spec helper or even in src/spec/dsl.cr to make it generally usable in specs.
• A datafile helper would just receive a path name relative to the data directory and turn it into a path relative to the project's root directory.

[RX14]

@straight-shoota tempfiles should be in a directory with a random name, so you can run multiple specsuites at once without clashing names.

[straight-shoota]

True. So with_tempfile should scope to $TMPDIR/crystal-XXXX.

[RX14]

@straight-shoota @tempdir ||= "$TMPDIR/crystal-spec-#{Random.hex(8)}/".

[straight-shoota]

I'm wondering if a datafile helper would actually be that useful. It would mean that "#{__DIR__}/data/datafile.ext" is replaced by datafile "datafile.ext". But it could also just be "spec/std/data/datafile.ext" instead. Is there any benefit from having the data path configurable? If need be, the path prefix can be easily grepped.

[RX14]

@straight-shoota the benefit would be that the spec suite would be $PWD agnostic, which helps a lot for cross-compiling the spec suite. Since the data path will vary between the compile host and the compile target.

[straight-shoota]

But can't we assume that specs should always be executed from the project root? So all paths could be relative to the project directory?

[RX14]

@straight-shoota Hmm, actually yeah I should just change my auto cross-compile script to run from a checkout of the crystal repo. So relative paths should actually be fine.

[straight-shoota]

While working on this, I noticed an actual benefit of datafile (or maybe better datapath): it is easier for defining paths platform-independent like datafile "dir", "f1.txt" instead of File.join("spec", "std", "data", "dir", "f1.txt"). This makes the specs DRYer and easier to read.