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

Add temp utilities (alias OS::get_temp_dir(), FileAccess::create_temp(), and DirAccess::create_temp()) #98397

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

adamscott
Copy link
Member

@adamscott adamscott commented Oct 21, 2024

This PR adds OS::get_temp_dir(), FileAccess::create_temp(), and DirAccess::create_temp() functions (the PR originally spelled tmp instead of temp), for all the supported platforms.

The concept of temporary files and directories

 There's already OS.get_cache_dir(), why do we need temporary files?

Temporary files and cache files are really similar, but they are distinct.

Cache files Temporary files
Can be deleted at any time by the user
Can be deleted at any time by the OS
Purpose Persistent temporary files, usually can be expected to be there the next time, useful after creation.  Volatile temporary files, usually not needed after use.
Example  Generated thumbnail icons. One-time write to a file to get the binary data of it.

So this is why Godot should support and provide an API for this special kind of files.

Methods

OS.get_temp_dir()

This function returns the global OS temporary dir for the application. It may be the root /tmp directory for most of the Unix/Linux systems, but for other platforms, it is the temporary dir given by the OS.

If no such thing as a temporary directory exist for the OS (ie. the Android platform), a directory inside the cache directory is chosen.

FileAccess.create_temp(int mode_flags, String prefix = "", String extension = "", bool keep = false)

This static function returns a FileAccess object with a temporary file open.

For sorting purposes, you can set prefix to the name of your project, just to be able to quickly debug temporary files if needed.

As ResourceLoaders usually detect the type of the file with the extension, you can pass a custom extension to add to the file name.

And if you set keep to true, the file will not get cleaned up when the FileAccess instance will be freed.

DirAccess.create_temp(String prefix = "", bool keep = false)

This static function returns a DirAccess object with a temporary directory open. Useful if you need temporary named files and directories.

prefix and keep exist such as in FileAccess.create_temp().

If keep is false, it will recursively delete the contents of the temporary directory when the DirAccess instance will be freed.

@adamscott adamscott force-pushed the add-tmp-support branch 2 times, most recently from 06160c8 to 0c00c5a Compare October 22, 2024 13:01
Calinou

This comment was marked as resolved.

@adamscott adamscott force-pushed the add-tmp-support branch 2 times, most recently from 46e4d80 to 9496552 Compare October 22, 2024 14:01
@Calinou
Copy link
Member

Calinou commented Oct 22, 2024

I've tried to use the following code, but despite create_tmp() being static, I get an error:

func _ready() -> void:
	var tmp_file := FileAccess.create_tmp(FileAccess.WRITE_READ, ".txt")
	tmp_file.store_string("I am *not* persisted after the file is closed.")
	tmp_file.close()
	OS.shell_open(tmp_file.get_path_absolute())  # Should fail because the file has been removed at this point.

	var tmp_file_keep := FileAccess.create_tmp(FileAccess.WRITE_READ, ".txt", true)
	tmp_file_keep.store_string("I am persisted after the file is closed.")
	tmp_file.close()
	OS.shell_open(tmp_file_keep.get_path_absolute())  # Should work because `keep` is `true`.
Cannot call method 'store_string' on a null value.

PS: Do modes other than FileAccess.WRITE make sense here? The file path is guaranteed to not exist before creation, so I don't think other FileAccess modes make sense to use here.

@adamscott
Copy link
Member Author

@Calinou The cleanup is done on the FileAccess destructor. So, .close() doesn't clean-up the file per se.

I did this because FileAccess::reopen() exists, but I just saw that it is not exposed to the public API (_bind_methods()).

@adamscott

This comment was marked as resolved.

@adamscott adamscott force-pushed the add-tmp-support branch 2 times, most recently from d480cc4 to 152e24c Compare October 22, 2024 15:57
@adamscott
Copy link
Member Author

PS: Do modes other than FileAccess.WRITE make sense here? The file path is guaranteed to not exist before creation, so I don't think other FileAccess modes make sense to use here.

What I did is that I create the file first with WRITE, then close, then open it again with the mode wished.

@Alex2782
Copy link
Member

https://chat.godotengine.org/channel/android?msg=3peujMoKbDJCQ0eul

Todo: Android bindings for OS::get_tmp_dir()

https://github.com/Alex2782/godot/tree/get_tmp_dir_for_android

5 files changed, +41 lines: Alex2782@9dd4af4

func _ready() -> void:
	print("get_cache_dir(): ", OS.get_cache_dir())
	print("get_tmp_dir(): ", OS.get_tmp_dir())

image

Tested on Android 13, Samsung Tab S7. I'm still not sure if I've taken everything into account correctly.

@adamscott adamscott force-pushed the add-tmp-support branch 3 times, most recently from 3f1822b to cb553c1 Compare October 23, 2024 15:23
@adamscott adamscott force-pushed the add-tmp-support branch 2 times, most recently from 5a7f5fe to e69feae Compare October 25, 2024 16:03
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally on Linux, it works as expected.

const ITERATIONS = 1_000

func _ready() -> void:
	for __ in ITERATIONS:
		FileAccess.create_tmp(FileAccess.WRITE_READ, "hello", "txt", true)
		DirAccess.create_tmp("world", true)

After checking the contents of /tmp, there are 1,000 files and folders created respectively.

@adamscott adamscott modified the milestones: 4.x, 4.4 Oct 27, 2024
@akien-mga
Copy link
Member

akien-mga commented Nov 11, 2024

We try to avoid abbreviations in the API, so we should probably avoid tmp throughout, even if that's the canonical Unix name. On Windows it would be temp which is at least bit better to the uninitiated.

We could go all the way with temporary, but since we do use dir already as an abbreviation in some of these APIs, I think something like OS::get_temp_dir / DirAccess::create_temp_dir / FileAccess::create_temp_file (note I'm adding _dir and _file for the DA/FA APIs, which I think are clearer than just create_tmp).

Thoughts?

@Mickeon
Copy link
Contributor

Mickeon commented Nov 11, 2024

temp is a common shorthand in programming to name anything temporary. Namely variables, but also game assets, in a similar vein to dummy.
So I think it's reasonably appropriate, but it's also worth noting there's no existing method in the API with "temp" or "temporary" in their name. The closest thing is the {temp_dir} placeholder for SSH Remote Deploy scripts.

@Repiteo
Copy link
Contributor

Repiteo commented Nov 12, 2024

temp is only slightly more verbose than tmp, and conveys the exact same information that temporary would. And while temp isn't used by any of our API, it IS used by the Python standard library tempfile which we import in some of our buildscripts (and all of its functions/variables). My vote is for temp.

@adamscott adamscott changed the title Add tmp utilities (alias OS::get_tmp_dir(), FileAccess::create_tmp(), and DirAccess::create_tmp()) Add tmp utilities (alias OS::get_temp_dir(), FileAccess::create_temp(), and DirAccess::create_temp()) Nov 18, 2024
@adamscott
Copy link
Member Author

I added a commit that renames tmp to temp! Thanks for the input everyone.

@adamscott adamscott changed the title Add tmp utilities (alias OS::get_temp_dir(), FileAccess::create_temp(), and DirAccess::create_temp()) Add temp utilities (alias OS::get_temp_dir(), FileAccess::create_temp(), and DirAccess::create_temp()) Nov 18, 2024
@Repiteo Repiteo requested a review from akien-mga November 19, 2024 19:29
@akien-mga
Copy link
Member

I would still suggest create_temp_file and create_temp_dir for the respective FileAccess and DirAccess APIs. But it's not a big blocker if others prefer just create_temp for both.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm

@Repiteo
Copy link
Contributor

Repiteo commented Nov 19, 2024

I prefer the plain create_temp for both, as it's more in-line with their existing api create and create_for_path.

@adamscott adamscott force-pushed the add-tmp-support branch 2 times, most recently from 90d7d3d to a025ca0 Compare November 26, 2024 19:06
@akien-mga akien-mga requested a review from bruvzg November 26, 2024 19:19
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on macOS, iOS and Windows, works as expected.

Co-authored by @Alex2782 for the Android bindings.
Many thanks to the reviewers also.

Co-authored-by: Alex <alex.hart.278@gmail.com>
@Repiteo Repiteo merged commit 156bc92 into godotengine:master Dec 3, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 3, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

9 participants