feature request: add support for keeping TempDir

[ethanpailes]

I would like to be able to conditionally avoid cleaning up a TempDir, and right now there doesn't seem to be an option for that besides resorting to shenanigans to avoid dropping the struct, which would likely lead to some memory leaks. It would be nice to add a disown() method that lets you mark a TempDir or NamedTempFile as not needing to be cleaned up on distruction.

My use case is that I'm using a TempDir as a place to throw some logs during tests and I want to be able to inspect those logs after the fact if I'm iterating on the test by setting an environment variable or something.

[ethanpailes]

Actually, I see now that there is already a keep method for NamedTempFile, so just adding keep support to TempDir would cover this request.

[Stebalien]

This is already supported through TempDir::into_path. The name difference comes from the fact that TempDir was merged in from an existing crate.

I'd add an alias (a new TempDir::keep function) that just calls TempDir::into_path, and deprecate into_path.

[ethanpailes]

It looks like path()/keep() is not quite equivalent to into_path() because

{
    let p = temp_dir.path().clone();
    temp_dir.keep();
    p
}

requires a copy while temp_dir.into_path() does not.

[ethanpailes]

I'll leave any deprecation marker up to you since it seems like they are not quite equivilant.

[Stebalien]

No? keep() returns the path.

[Stebalien]

Well:

• NamedTempFIle::keep returns the file and path.
• TempPath::keep returns the path.

[abrisco]

Any updates here? I'm interested in in making the change but want to know if there's a recommended path forward. My thoughts would be to add some keep function that would toggle a flag and prevent the directory from being removed when the instance is dropped. Does that sound reasonable?

[Stebalien]

@abrisco you can already "keep" the temporary directory by calling into_path(). The discussion here is mostly about naming.

Does this not work for you?

[abrisco]

That will prevent the temp directory from being cleaned up? Is there a test for that?

[abrisco]

Neat! https://docs.rs/tempfile/latest/tempfile/struct.TempDir.html#method.into_path seems intentional 😄 thanks for the info!

[RalfJung]

Sadly into_path doesn't cover this usecase fully. The idea is that usually we want everything to be cleaned up, but sometimes we need to inspect what went wrong with a test, and then we want the directories to stick around. It's not uncommon for tools to have flags or environment variables that say "do not remove the tempdirs, I want to inspect them to see what went wrong". I'd like to add such a flag to the test suite I am working on as well, but using TempDir that's pretty tricky.

Sadly with into_path it's not possible to achieve this, since it changes the type -- so I can't do it base on a dynamic condition. I think this needs some sort of function that takes &mut TempDir and changes the internal state, so that we can dynamically choose whether to keep or clean up the tempdir.

[ethanpailes]

You can still dynamically do that using into_path, though it might be a little clunky. At the very least you can just keep a PathBuf that you either copy from TempDir.path() or get from into_path. Then keep an Option<TempDir> alongside to keep the directory live in the cleanup case (set it to none of you use into_path).

…

On Sat, Mar 9, 2024 at 4:41 AM, Ralf Jung ***@***.***(mailto:On Sat, Mar 9, 2024 at 4:41 AM, Ralf Jung <<a href=)> wrote: Sadly into_path doesn't cover this usecase fully. The idea is that usually we want everything to be cleaned up, but sometimes we need to inspect what went wrong with a test, and then we want the directories to stick around. It's not uncommon for tools to have flags or environment variables that say "do not remove the tempdirs, I want to inspect them to see what went wrong". I'd like to add such a flag as well, but using TempDir that's pretty tricky. Sadly with into_path it's not possible to achieve this, since it changes the type -- so I can't do it base on a dynamic condition. I think this needs some sort of function that takes &mut TempDir and changes the internal state, so that we can dynamically choose whether to keep or clean up the tempdir. — Reply to this email directly, [view it on GitHub](#194 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ABA3DZJSHRABT6ASRN2IY5LYXLKLHAVCNFSM6AAAAAARKD36NOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBWHAYDQOJTGM). You are receiving this because you authored the thread.Message ID: ***@***.***>

[RalfJung]

Yeah I guess it's possible but pretty inconvenient and also not very self-explaining.

let tempdir = tempfile::tempdir()?;
let (_tempdir, path) = if keep_files {
  (None, tempdir.into_path())
} else {
  let path = tempdir.path().to_owned();
  (Some(tempdir), path)
};

[RalfJung]

So you consider the above to be resolving this feature request? That's unfortunate, it makes the regularly needed pattern of "provide a flag to keep the temp files" pretty hard to express and hard to read as well. (Such a flag doesn't exist nearly as often as it should, but when it existed it has saved be tons of time already. It would probably exist more often if it wasn't so hard to implement...)

[ethanpailes]

I’m not the maintainer, I just opened this fr. If I was the maintainer I would probably add a keep method that mutates the strict in place, but the functionality is technically achievable today so I decided to close the issue. I seems like you still want it addressed so I can re-open.

[RalfJung]

Thanks :) Am 14. März 2024 15:03:50 UTC schrieb Ethan Pailes ***@***.***>:

…

I’m not the maintainer, I just opened this fr. If I was the maintainer I would probably add a keep method that mutates the strict in place, but the functionality is technically achievable today so I decided to close the issue. I seems like you still want it addressed so I can re-open. -- Reply to this email directly or view it on GitHub: #194 (comment) You are receiving this because you commented. Message ID: ***@***.***>

[Stebalien]

I'd like to narrow down the use-case here. @RalfJung do you need to:

1. As with the original request, launch a program with temporary file/directory cleanup disabled (e.g., via an environment variable).
2. After the fact, keep a single temporary file/directory.
3. After the fact, keep all temporary files/directories (e.g., when something fails).

[RalfJung]

The case that came up for me is (1) -- when the TempDir is constructed, I know whether I want cleanup or not, but it's a runtime decision.

[Stebalien]

So you want a program-wide runtime decision? I.e., a global RUST_TEMPFILE_KEEP=1 wouldn't be sufficient?

[RalfJung]

I generally consider "library configured by env var" to be an anti-pattern; the way this is exposed to the user should be up to higher-level crates. Even if one accepts "let's just use global mutable state for configuration" (which goes against all Rust design principles), there's the problem that set_var will have to become unsafe eventually, so just providing an env-var-based API is not sufficient to let safe code control this behavior.

[Stebalien]

They're both bad, but an env-var is better as it's ultimately the user's decision decision and makes it clear that libraries shouldn't mess with it. It's similar to the RUST_BACKTRACE environment variable in that way.

[RalfJung]

The RUST_BACKTRACE variable is in the progress of getting a sister API that lets a program control this without env::set_var. All libraries that rely on env vars should provide such APIs. It's also a special case with it being a core part of the Rust runtime. Already RUST_LOG, which is a lot more widely used than this crate, provides full control over the env var choice to downstream crates and lets them overwrite the value as well (without set_var). RUST_LOG is merely a convenient default constructor.

For this crate to work like RUST_LOG would require significant API surface. That seems overkill for me when a single bool somewhere, or a new keep method on the builder, is sufficient.

it's ultimately the user's decision

When I write an application it should be my choice how to expose "keep the files". Maybe I want a --keep CLI flag, not an env var.

[Stebalien]

I do not like global toggles, period. But I'm willing to compromise as long as there are restrictions (even if they're just "you should know better" restrictions) on when and by whom it can be set.

I'm not so concerned about the main program setting it, I'm concerned about libraries setting it and messing with the behavior of other libraries and/or the main program. With RUST_BACKTRACE, the behavior is only observable on panic and it doesn't really change the behavior of the program. In this case, setting this flag will affect the behavior of the entire program.

I only suggested this because you answered (1) to #194 (comment).

That seems overkill for me when a single bool somewhere, or a new keep method on the builder, is sufficient.

You're right, a per-tempfile keep flag would be much better if that's sufficient for your use-case.

On the other hand, have you considered a MaybeTempDir enum? Having a "psych, I'm not actually a temporary TempDir option" is very strange, so I'd rather not implement it unless implementing such an enum would actually be a problem for some reason.

[ethanpailes]

A per temp file keep flag makes the most sense to me. A global toggle seems like a bad idea however it is controlled.

…

On Fri, Mar 15, 2024 at 9:56 AM, Steven Allen ***@***.***(mailto:On Fri, Mar 15, 2024 at 9:56 AM, Steven Allen <<a href=)> wrote: I do not like global toggles, period. But I'm willing to compromise as long as there are restrictions (even if they're just "you should know better" restrictions) on when and by whom it can be set. I'm not so concerned about the main program setting it, I'm concerned about libraries setting it and messing with the behavior of other libraries and/or the main program. With RUST_BACKTRACE, the behavior is only observable on panic and it doesn't really change the behavior of the program. In this case, setting this flag will affect the behavior of the entire program. I only suggested this because you answered (1) to [#194 (comment)](#194 (comment)). > That seems overkill for me when a single bool somewhere, or a new keep method on the builder, is sufficient. You're right, a per-tempfile keep flag would be much better if that's sufficient for your use-case. On the other hand, have you considered a MaybeTempDir enum? Having a "psych, I'm not actually a temporary TempDir option" is very strange, so I'd rather not implement it unless implementing such an enum would actually be a problem for some reason. — Reply to this email directly, [view it on GitHub](#194 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ABA3DZNKRHMAJTITQXW2AUDYYL42HAVCNFSM6AAAAAARKD36NOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJZG4ZDEMZVGQ). You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[Stebalien]

Fair enough. But then the question is, why not:

enum MaybeTempDir {
    Temporary(TempDir),
    Keep(PathBuf),
}

impl MaybeTempDir {
    fn new() -> {
        // my crate-specific keep or don't keep logic.
    }
}

impl Deref for MaybeTempDir {
    type Target = Path;
    fn deref(&self) -> &Path {
        match self {
            Temporary(p) => &p,
            Keep(p) => &p,
        }
    }
}

I.e., would implementing a wrapper like this significantly increase complexity, infect your APIs, etc.?

[RalfJung]

I do not like global toggles, period. But I'm willing to compromise as long as there are restrictions (even if they're just "you should know better" restrictions) on when and by whom it can be set.

I don't like them either. I'm not asking for a global toggle. You suggested RUST_TEMPFILE_KEEP which I would call a global toggle. Maybe we just misunderstood each other here.

That MaybeTempDir enum is an interesting idea. It's definitely better than the sketch above, though unfortunately it also takes twice as much code. It's a misnomer though, it's still always a temp dir, just sometimes with and sometimes without automatic cleanup.

I just think that's a core functionality of a temporary directory type: letting me control automatic cleanup. It'd be quite unfortunate to have to come up with this idea, and write 20 lines of code, just to do some debugging of my application's temp dir state.

[Stebalien]

Ok, it sounds like we're on the same page. I on;y suggested RUST_TEMPFILE_KEEP because that was the original request.

For me, having the option to disable temporary file cleanup seems a bit weird but... well, Python lets you do it (https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory) and temporary directory's in C & Go don't have automatic cleanup at all.

So yeah, a keep(bool) or cleanup(bool) function on the builder seems reasonable. The overhead is relatively low anyways.

Thanks for bearing with me on this. Can you submit a PR? I'm fine with keep, cleanup, dont_remove, or anything inbetween.

[Stebalien]

Note: we should apply this feature to both temporary files and temporary directories, for symmetry.

[RalfJung]

Ok, it sounds like we're on the same page. I on;y suggested RUST_TEMPFILE_KEEP because that was the original request.

👍

Thanks for bearing with me on this. Can you submit a PR? I'm fine with keep, cleanup, dont_remove, or anything inbetween.

I can add it to my todo list. It's a long list though... so if anyone else reading along wants to pick this up, don't hesitate. :)