feature request: add support for keeping TempDir #194

ethanpailes · 2022-10-20T12:51:07Z

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.

The text was updated successfully, but these errors were encountered:

ethanpailes · 2022-10-20T13:16:45Z

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

This patch adds keep() support to TempDir. Rather than using mem::forget() tricks like NamedTempFile does, it just uses a flag, which I think is a little easier to follow, though does require an extra byte of memory footprint. Closes Stebalien#194

Stebalien · 2022-10-20T20:25:06Z

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 · 2022-10-21T17:51:48Z

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.

This patch adds a keep() method to TempDir so that it better matches the interface of NamedTempFile. Closes Stebalien#194

ethanpailes · 2022-10-21T17:54:06Z

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

Stebalien · 2022-10-21T19:21:59Z

No? keep() returns the path.

Stebalien · 2022-10-21T19:24:02Z

Well:

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

This patch adds a keep() method to TempDir so that it better matches the interface of NamedTempFile. Closes Stebalien#194

abrisco · 2023-05-14T20:58:18Z

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 · 2023-05-16T19:17:21Z

@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 · 2023-05-16T23:35:15Z

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

abrisco · 2023-05-17T12:07:25Z

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

RalfJung · 2024-03-09T09:40:45Z

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 · 2024-03-09T15:22:49Z

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 · 2024-03-09T16:07:39Z

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 · 2024-03-14T14:51:15Z

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 · 2024-03-14T15:03:28Z

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 · 2024-03-14T15:11:24Z

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 · 2024-03-14T15:38:24Z

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

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

RalfJung · 2024-03-14T20:31:26Z

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 · 2024-03-14T20:35:59Z

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

RalfJung · 2024-03-14T21:04:27Z

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 · 2024-03-14T22:28:33Z

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 · 2024-03-15T06:58:47Z

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 · 2024-03-15T13:56:28Z

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 · 2024-03-15T16:37:55Z

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 · 2024-03-15T17:54:43Z

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 · 2024-03-17T12:19:36Z

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 · 2024-03-18T18:11:56Z

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 · 2024-03-18T18:14:57Z

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

RalfJung · 2024-03-18T18:31:03Z

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. :)

ethanpailes changed the title ~~feature request: add support for disowning TempDir/NamedTempFile~~ feature request: add support for keeping TempDir Oct 20, 2022

ethanpailes mentioned this issue Oct 20, 2022

add keep support to TempDir #195

Closed

ethanpailes added a commit to ethanpailes/tempfile that referenced this issue Oct 21, 2022

add keep support to TempDir

0ca6793

This patch adds a keep() method to TempDir so that it better matches the interface of NamedTempFile. Closes Stebalien#194

ethanpailes mentioned this issue Oct 21, 2022

add keep support to TempDir #197

Closed

ethanpailes added a commit to ethanpailes/tempfile that referenced this issue Oct 31, 2022

add keep support to TempDir

68d8484

This patch adds a keep() method to TempDir so that it better matches the interface of NamedTempFile. Closes Stebalien#194

ethanpailes closed this as completed Mar 14, 2024

ethanpailes reopened this Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature request: add support for keeping TempDir #194

feature request: add support for keeping TempDir #194

ethanpailes commented Oct 20, 2022

ethanpailes commented Oct 20, 2022

Stebalien commented Oct 20, 2022

ethanpailes commented Oct 21, 2022

ethanpailes commented Oct 21, 2022

Stebalien commented Oct 21, 2022

Stebalien commented Oct 21, 2022

abrisco commented May 14, 2023

Stebalien commented May 16, 2023

abrisco commented May 16, 2023 •

edited

abrisco commented May 17, 2023

RalfJung commented Mar 9, 2024 •

edited

ethanpailes commented Mar 9, 2024 via email

RalfJung commented Mar 9, 2024 •

edited

RalfJung commented Mar 14, 2024 •

edited

ethanpailes commented Mar 14, 2024

RalfJung commented Mar 14, 2024 via email

Stebalien commented Mar 14, 2024

RalfJung commented Mar 14, 2024

Stebalien commented Mar 14, 2024

RalfJung commented Mar 14, 2024 •

edited

Stebalien commented Mar 14, 2024

RalfJung commented Mar 15, 2024 •

edited

Stebalien commented Mar 15, 2024

ethanpailes commented Mar 15, 2024 via email

Stebalien commented Mar 15, 2024

RalfJung commented Mar 17, 2024 •

edited

Stebalien commented Mar 18, 2024

Stebalien commented Mar 18, 2024

RalfJung commented Mar 18, 2024

feature request: add support for keeping TempDir #194

feature request: add support for keeping TempDir #194

Comments

ethanpailes commented Oct 20, 2022

ethanpailes commented Oct 20, 2022

Stebalien commented Oct 20, 2022

ethanpailes commented Oct 21, 2022

ethanpailes commented Oct 21, 2022

Stebalien commented Oct 21, 2022

Stebalien commented Oct 21, 2022

abrisco commented May 14, 2023

Stebalien commented May 16, 2023

abrisco commented May 16, 2023 • edited

abrisco commented May 17, 2023

RalfJung commented Mar 9, 2024 • edited

ethanpailes commented Mar 9, 2024 via email

RalfJung commented Mar 9, 2024 • edited

RalfJung commented Mar 14, 2024 • edited

ethanpailes commented Mar 14, 2024

RalfJung commented Mar 14, 2024 via email

Stebalien commented Mar 14, 2024

RalfJung commented Mar 14, 2024

Stebalien commented Mar 14, 2024

RalfJung commented Mar 14, 2024 • edited

Stebalien commented Mar 14, 2024

RalfJung commented Mar 15, 2024 • edited

Stebalien commented Mar 15, 2024

ethanpailes commented Mar 15, 2024 via email

Stebalien commented Mar 15, 2024

RalfJung commented Mar 17, 2024 • edited

Stebalien commented Mar 18, 2024

Stebalien commented Mar 18, 2024

RalfJung commented Mar 18, 2024

abrisco commented May 16, 2023 •

edited

RalfJung commented Mar 9, 2024 •

edited

RalfJung commented Mar 9, 2024 •

edited

RalfJung commented Mar 14, 2024 •

edited

RalfJung commented Mar 14, 2024 •

edited

RalfJung commented Mar 15, 2024 •

edited

RalfJung commented Mar 17, 2024 •

edited