Looks good! Could you add a little doc for datapath, with_tempfile and SPEC_TEMPFILE_CLEANUP to explain when and how to use them?

Damn, the temp paths are too long for socket_spec because unix socket paths are limited to 103 bytes on OSX. Linux works though, it seems to have 108 bytes.

@bew I added a comment to with_tempfile. It might even make sense to move this to the standard library as this feature would certainly be useful for other spec suites as well.

datapath is self-explanatory, I don't think it needs a comment.

On OSX it seems that $TEMPDIR it a randomized folder in /var/folders/ (like /var/folders/ms/xg67k5sn16xc7sdr_w3q45840000gn/T/ in one case). That's particularly bad for unix socket paths because together with the folder structure added by with_tempfile that's already 78 of maximum 103 bytes: /var/folders/ms/xg67k5sn16xc7sdr_w3q45840000gn/T/crystal-spec-12345678/socket/.

Any ideas on this? If we just set $TEMPDIR to /tmp in the ci script it should be fine, CI doesn't need user-specific temp dirs. Otherwise we could let the socket specs use custom temp paths. Or simply use shorter file names in the spec to stay below the limit?

Apart from the OSX issue this is mostly complete. The remaining instances of __DIR__ and __FILE__ in spec are used as compile time resources such as file location or parser tokens.

crystal_path_spec still needs to be reworked, I have to look into the specs and implementation to figure out how exactly it is supposed to work. I wouldn't mind if someone had some insight on that (or even take over that part).

ecr_spec still uses hard coded paths to #{__DIR__}/../data/. This isn't bad in itself, because they are used at compile-time and can't use datapath for this. But I guess the paths could at least be hard coded relative to project root instead of __DIR__.

It would be beneficial if this PR could get reviewed and hopefully merged soon. It touches a lot of code in the spec suite, but it's mostly insignificant changes. I'd like to avoid lots of upcoming rebasing in this and future PRs.

Any idea on the OSX issue @RX14?

If TMPDIR is some weird path on osx, just ignore it and use /tmp.

@RX14 Do you mean generally, in with_tempfile (and if so, should it always switch to /tmp if it is /var/folders?) or just use custom temp file paths in Unix socket specs?

IMHO the cleanest solution would be to set TMPDIR to /tmp in the ci script because it keeps platform specifics out of the spec code.

IMHO the cleanest solution would be to set TMPDIR to /tmp in the ci script because it keeps platform specifics out of the spec code.

Fixing things in the CI script is always wrong because you're admitting you're going to break the specs on everyone's osx laptops.

I meant change Tempfile.tempdir itself :)

I meant change Tempfile.tempdir itself :)

But why? If a user consciously sets TMPDIR to a specific path (or doesn't change the OS default but the difference is hard to tell), we should not just overwrite that. I don't think that makes any sense.
Consider the case that two programs on OSX are configured to connect to $TMPDR/mysocket.sock and TMPDIR is /var/folders/ms/xg67k5sn16xc7sdr_w3q45840000gn/T. If one is a Crystal program and ignores TMPDIR, they won't connect even if the path is actually valid. And that's hard to debug. If the path would be too long, it's even more confusing: The Crystal program would create a socket without issues but any other program complains about the path.

We could think about overwriting the temp dir used by with_tempfile. That can either be done in the spec code (which I don't like) or by configuring TMPDIR appropriately.

You could see it that way: The specs for Unix sockets puts them in a sub path of TMPDIR. In order for that to proceed, a short TMPDIR path is required. That's just an factual, external requirement for running the stdlib's spec suite. But certainly not for every Crystal program.

Let's consider the downsides: If we change the behaviour of Dir.tempdir to partially ignore TMPDIR, this will be highly unexpected (see example above) and for the great majority of use cases it is completely unnecessary. When TMPDIR path is too long, it can cause problems with Unix sockets. That's a fact, and we can't change it.

If we set TMPDIR in the ci script, the worst thing that can happen is that Unix socket specs fail on some people's OSX laptops. That's not a real problem and they can easy fix it by setting TMPDIR to a shorter path. We could even consider setting TMPDIR in the Makefile or bin wrapper to get it right for everyone's OSX laptop as well. Again: It's a specific requirement of the stdlib spec suite, not for every Crystal program.

However, this absolutely needs to be documented. Unix sockets will also fail in applications using a TMPDIR that is too long - and that's on any system, not just OSX.

Changing the spec suite to adapt to a special case is not a good solution for a generic problem. We should rather improve the error message for Unix sockets failing because of path length. Instead of Path size exceeds the maximum size of 103 bytes, it should at least show the exact path. And as it is pretty common to put Unix sockets in a temp dir, it could even check if the path is in TMPDIR and suggest adjusting the environment variable.

Tempfile.dirname isn't documented or specified to be TMPDIR, just a tempoary directory. If your program relies on it being TMPDIR, you can use ENV["TMPDIR"].

Specs failing is very much a "real problem"...

It's not documented but I'd say it's just missing documentation. The specs for Tempfile.dirname explicitly test for ENV["TMPDIR"] so it's part of the public API definition and looks to be intended (and expected) behaviour: if I create a Tempfile I expect the path to be in TMPDIR.

And again, I see no sane reason to generally ignore the environment variable if we think it's to long for one specific use case.

Let's just fix it by hard coding to /tmp on osx in with_tempfile

I really don't like that with_tempfile should override TMPDIR when it is specified. I would much rather prefer to set TMPDIR appropriately for running specs.

To get this moving forward, what do others think?

/cc @crystal-lang

well, the fact is that $TMPDIR on macos can be set to an insane value. How do you possibly intend on returning to sanity while respecting $TMPDIR?

1. Expect users to have a sane $TMPDIR when using Unix sockets (including but not limited to stdlib specs). This doesn't affect only MacOS: When a Linux user has a long $TMPDIR, Unix sockets will fail as well. There is nothing we can do about that unless we want to dictate everyone where their $TMPDIR should be.
2. Add export TMPDIR=/tmp to bin/ci so that the spec suit is run with a sane environment setting.

I use bin/crystal run spec/std/xyz_spec.cr all the time, I refuse to merge any implementation that breaks doing that on osx. Which means either: reset $TMPDIR in bin/crystal (:-1:), or ignore $TMPDIR entirely because macos is fucked.

Note i'm assuming that people using osx have a really long $TMPDIR outside of circleci.

It would only break if the Unix socket specs are run and $TMPDIR is too long. That's unfortunately the default on OSX but it can easily be changed.

There are countless ways to do that. A few suggestions: change global config, apply context-sensitive in crystal project folders, explicitly export in the current session, manually prepend TMPDIR=/tmp or just use an alias such as bin-crystal with TMPDIR=/tmp bin/crystal $@. I think there should be a feasible solution for everyone.

And I'd improve the error message for Unix sockets failing because of path length, including an advice to change TMPDIR if the socket path is in that folder (see last paragraph of #5951 (comment)).

@RX14 I'll try it with a shorter tempfile subpath. Let's see if it works.

$ echo -n '/var/folders/ms/xg67k5sn16xc7sdr_w3q45840000gn/T/cr-spec-12345678/socket/unix_server-accept-closed.sock' | wc -c
103

if @path.bytesize + 1 > MAX_PATH_SIZE

we're one off.

Merge? (before this PR needs yet another rebase...)

I think that after this PR the make spec is broken. The ci uses make compiler_spec and make std_spec independently so it went unnoticed.

$ make crystal spec
Using /usr/local/bin/llvm-config [version=5.0.1]
CRYSTAL_CONFIG_PATH="/path/to/crystal" ./bin/crystal build  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
.build/all_spec 
Error: File spec/std/data/compiler_sample does not exist

True. The datapath definition from std spec helper overwrites the one from compiler spec.
We could easily rename it to datapath_compiler. Or just split make spec into compiler_spec and std_spec. I don't think it makes much sense to combine compiler and std specs into a single spec run/binary. They're actually independent spec suits.

I disagree. It's faster to compile combined.

I think we should have 2 datapath helpers also.