Skip to content
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

We need better temporary file and dir API #9372

Closed
2 tasks done
timotheecour opened this issue Oct 14, 2018 · 15 comments · Fixed by #17361
Closed
2 tasks done

We need better temporary file and dir API #9372

timotheecour opened this issue Oct 14, 2018 · 15 comments · Fixed by #17361

Comments

@timotheecour
Copy link
Member

timotheecour commented Oct 14, 2018

  • getTempDir ; EDIT: solved by better getTempDir #16914
    getTempDir comes with a warning Please do not use this and indeed its implementation should be replaced (or alternatively, deprecated for another proc if backward compatibility is an issue) by something more robust.
  • temporary file and dir creation; EDIT: see close #9372 add std/tempfiles #17361
    There's, in posix, proc mkstemp*(tmpl: cstring): cint but that's posix only and too low-level (eg see accompanying warning to it)

Shopping around for alternatives, python has nice standard library library with low level and high level API around temp file/dir creation, including facilities for cleanup, and with epmhasis on security (eg see description around TemporaryFile).

We could start with porting these:

tempfile.mkstemp(suffix=None, prefix=None, dir=None, text=False)
# returns a tuple containing an OS-level handle to an open file (as would be returned by os.open()) and the absolute pathname of that file, in that order.

tempfile.mkdtemp(suffix=None, prefix=None, dir=None)

Note: the D variant (https://dlang.org/library/std/stdio/file.tmpfile.html) is not as convenient as it returns a File object (and no way to access its name)

links

no direct cross-platform convenience mktemp-like method yet

@haltcase
Copy link
Contributor

Probably worth noting there's already a nimble package for this. https://github.com/OpenSystemsLab/tempfile.nim

@timotheecour
Copy link
Member Author

ah, thanks! but definitely some points in this issue should be addressed. in the eternal debate of 'batteries included or not', I feel temp file creation should be part of stdlib because it's generally useful AND because it's likely to be used in Nim repo itself (eg a lot of tests should use tempfile creation instead of what's being done currently:

$nimc_D/tests/stdlib/tparsecfg.nim
config.writeConfig("test.ini")
which dumps to current working dir and complicates .gitignore

@timotheecour
Copy link
Member Author

note: https://github.com/OpenSystemsLab/tempfile.nim has limitations, eg:

ba0f3 added a commit to OpenSystemsLab/tempfile.nim that referenced this issue Dec 2, 2018
@hyao
Copy link

hyao commented May 20, 2019

Python tempfile implementation:
https://docs.python.org/3.7/library/tempfile.html

@pb-cdunn
Copy link
Contributor

Ironically, nimble itself uses getTempDir().

@sschwarzer
Copy link
Contributor

sschwarzer commented Dec 10, 2019

* https://nim-lang.github.io/Nim/posix_utils#mkstemp,string
* https://nim-lang.github.io/Nim/posix_utils#mkdtemp,string

Yes, these seem to be Posix-only. I just find myself in the situation where I want to adapt a program that I developed under Linux to Windows and this mkdtemp limitation is a bit annoying.

@ghost
Copy link

ghost commented Dec 11, 2019

@sschwarzer yeah ok, but mkstemp works fine even on Windows. and if youre going that route, then youll need to throw out PHP and other too, as they can only create temp files and not directories either.

@sschwarzer
Copy link
Contributor

sschwarzer commented Dec 11, 2019

@sschwarzer yeah ok, but mkstemp works fine even on Windows. and if youre going that route, then youll need to throw out PHP and other too, as they can only create temp files and not directories either.

That's interesting, thank you. I had assumed that things in the posix module would only work on Posix platforms. Anyway, I need mkdtemp.

I agree with @timotheecour that creating temporary directories and files should be in the Nim standard library. I'll probably go with the external tempfile library for now that was mentioned above. I had considered working on mkdtemp and mkstemp in the standard library, but at the moment I don't have the time. :-/

(Regarding PHP, I haven't used it in years, although not specifically because of the temp directory omission but because I didn't have a project where I'd use PHP. And by the way, if I say that a feature should be in the standard library of a language that doesn't automatically mean I won't use the language until the standard library has the feature.)

@maxgrenderjones
Copy link
Contributor

maxgrenderjones commented May 14, 2020

I'm a git/github newbie so please be gentle, but have just opened #14347 to get support for mkstemps.

template withTempFile(prefix: string, suffix: string, code: untyped): untyped =
    let (filename, tempfile {.inject.}) = mkstemp(prefix, suffix)
    let tempfilename {.inject.} = absolutePath(filename)
    try:
        code
    finally:
        removeFile(tempfilename)

Is the user-friendly version of the same, but I don't know if that has the cross-platform support it would need to go into os. (I'm on OSX where the temp file seems to get created in the current directory, so I'm not sure what happens if TMPDIR is set to something else - I'm not sure what you need to do to get the absolutePath in a way that works on all OSes - generic way to get temp path suggests that you'd need to change the call to the posix mkstemp to ensure that the file is created a well-known place that honours TMPDIR. tempfile.py may be instructive (seems they just try a bunch of directories until one succeeds).

@kaushalmodi
Copy link
Contributor

May be https://github.com/OpenSystemsLab/tempfile.nim should be made part of fusion ( https://github.com/nim-lang/fusion ) if the author allows? /cc @OpenSystemsLab @ba0f3

@ghost
Copy link

ghost commented May 14, 2020

@kaushalmodi well with MIT you don't really need permission, just need to keep the license, but yeah, it would be a nice thing to ask :)

@ba0f3
Copy link
Contributor

ba0f3 commented May 14, 2020

@kaushalmodi why not 😀

@kaushalmodi
Copy link
Contributor

@ba0f3 Thanks. With your approval, now we need to figure out how to proceed. @Araq ?

@timotheecour
Copy link
Member Author

just create a PR against fusion ?

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

Successfully merging a pull request may close this issue.

10 participants