RFC: Use macros for separate functions on OS X and Linux

[johnmyleswhite]

To resolve the issues that came up in 3327b62, I've added macros that split apart OS X and Linux in the same way that we previously split apart Windows and Unix. For this specific case, we should ultimately not keep the resulting definitions of tempdir and tempfile, but I suspect that we will find separating OS X from Linux useful in other places as well.

Because this introduces new macros everywhere, I wanted to get people's opinions about this strategy before using it for such a small problem.

[timholy]

I'm tempted to just hit merge myself, but since one of my changes motivated this I should let at least one other core developer review this. First to do so wins the prize!

[pao]

Four nearly-identical one-argument macros sounds like a job for a two-argument macro and better abstraction.

macro os_only(os, ex)
    if os == this_os()
        ex
    else
        :nothing
    end
end

Then wrap the OS detection code in this_os() (name bikesheddable).

[johnmyleswhite]

I'm on board. But are the various Linux OS's being exposed as a consistent name? We'll also still need a hierarchy of names to express those functions that work on both OS X and Linux.

[pao]

I think the simple refactoring would be fine. I don't think we need to refactor the world yet; just DRYing this up would get the maintainability there and it should be good enough for now.

[StefanKarpinski]

This looks sane to me. I vote for merge first, dry after.

[johnmyleswhite]

Ok. I will merge this now, then DRY it after lunch.

[staticfloat]

I'm a little late to the party, but I recommend splitting not on Linux/OSX, but Linux/BSD, as these kinds of errors are typically because of OSX's BSD heritage, and the corresponding BSD commands usually have the same difference from Linux commands.

[pao]

macro os_only(oses, ex)
    if any(oses == this_os())
        ex
    else
        :nothing
    end
end

@os_only [:bsd, :osx] begin
    # platform specific code
end

? Should also address @johnmyleswhite's concern about code applicable to multiple platforms. An analogous @os_except could notch out a particular platform (coughWindowscough) as well.

[StefanKarpinski]

That works. I also just pushed a more concise version. Unfortunately, I'm unable to get tests to pass due to tempfile stuff that's failing.

[johnmyleswhite]

I had problems with tempfile stuff that crept in while revising the tests in test/file.jl. At one point the system failed. Once it fails once, it never works again because of how the first few lines in test/file.jl work.

Is that the source of your problems? What do tempfile() and tempdir() output for you?

[johnmyleswhite]

I'm happy to add these cleaner macros, but I'd like to make sure we have a handle on what OS's are causing problems for temporary files and directories before I do anything else.

[StefanKarpinski]

I disabled them for now. The error message is this:

mktemp: too few X's in template ‘tmp’
failed process: `mktemp -d -t tmp` [ProcessExited(1)]

[johnmyleswhite]

What OS (or version of OS X) are you on? That's the error that Tim Holy used to get, which led him to commit changes. Maybe on your machine you need to be running the linux_only version. Can you try that?

[johnmyleswhite]

Also, which mktemp is in your path? For me, it's /usr/bin/mktemp on 10.7.5 with command line tools from XCode 4.4.1.

[StefanKarpinski]

That's the issue – I'm using GNU mktemp:

$ type -a mktemp
mktemp is /opt/local/libexec/gnubin/mktemp
mktemp is /opt/local/bin/mktemp
mktemp is /usr/bin/mktemp

[StefanKarpinski]

So I guess one solution would be to hardcode the absolute path on OS X.

[johnmyleswhite]

Looking at the source of http://www.mktemp.org/, I'm starting to think we should just generate a random string and create things inside of ENV["TMPDIR"]. It should be easy to make a minimal version of this in pure Julia.

[johnmyleswhite]

On another note, are we trying to get rid of all the previous OS detection stuff? If we're cutting out the linux_only and osx_only along with the unix_only and windows_only macros and replacing them all with

macro os_only(oses, ex)
    if any(oses == OS_NAME)
        ex
    else
        :nothing
    end
end

we'll need to revise a lot of other places. Or is the intention to keep unix_only and windows_only, then provide os_only as the alternative when greater specificity is needed?

[timholy]

There's already a randstring function, which I wrote (Jeff later improved) precisely for this use case. I thought about suggesting it earlier, but we'd have to make certain that two separate Julia sessions wouldn't generate the same strings.

[johnmyleswhite]

This is a case where being able to set the seed for a local RNG without effecting the global one would be really helpful.

[johnmyleswhite]

Does something like this work on other people's systems?

function new_tempfile()
  filename = file_path(ENV["TMPDIR"], randstring(8))
  while isfile(filename)
    filename = file_path(ENV["TMPDIR"], randstring(8))
  end
  try
    f = open(filename, "w")
    close(f)
  catch
    error("Unable to create tempfile: $filename")
  end
  return filename
end

[staticfloat]

Works great on OSX 10.8.2, generates /var/folders/tz/hcrbln110h581dh1r04wgrqm0000gn/T//oTGisjAQ so there's a double slash at the end because ENV["TMPDIR"] contains a slash at the end, but that's functionally identical to the ideal case. I'll see if I can't fix up file_path() to strip out // terms like that.

[johnmyleswhite]

Good to know. I think something like this that removes our dependence on external programs like mktemp is a much better long-term approach. I sort of doubt this will work on Windows, but I suppose our current version didn't work there either.

[timholy]

Sadly, does not for me (Kubuntu 12.04, TMPDIR is not an environment variable).

We could put it in a try/catch block and default to /tmp, I suppose. And it would be nice to have something that would work for Windows, too. @vtjnash and @loladiro ?

[johnmyleswhite]

I guess the other option is to split things up based on the value of some version for mktemp. That seems like a terrible road to down, though.

[staticfloat]

I think @timholy has the right idea...... for reference, here's how Python does it:

def _candidate_tempdir_list():
    """Generate a list of candidate temporary directories which
    _get_default_tempdir will try."""

    dirlist = []

    # First, try the environment.
    for envname in 'TMPDIR', 'TEMP', 'TMP':
        dirname = _os.getenv(envname)
        if dirname: dirlist.append(dirname)

    # Failing that, try OS-specific locations.
    if _os.name == 'nt':
        dirlist.extend([ r'c:\temp', r'c:\tmp', r'\temp', r'\tmp' ])
    else:
        dirlist.extend([ '/tmp', '/var/tmp', '/usr/tmp' ])

    # As a last resort, the current directory.
    try:
        dirlist.append(_os.getcwd())
    except (AttributeError, _os.error):
        dirlist.append(_os.curdir)

    return dirlist

Although if we want to be good Windows citizens, we can do better than that. There's GetTempFileName on XP and above....