-
Notifications
You must be signed in to change notification settings - Fork 9
Separate cache dir from Maxima data dir #45
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
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| fn zstate_path(id: &str, path: &str) -> Result<PathBuf, DownloaderError> { | ||
| let mut path = maxima_dir()?.join("temp/downloader").join(id).join(path); | ||
| let mut path = maxima_cache_dir()?.join("downloader").join(id).join(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this to be in cache and not in the temp directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about /tmp, since it's usually tmpfs so only tiny files can be put there; also might be more hassle:
Since
/tmp/is accessible to other users of the system, it is essential that files and subdirectories under this directory are only created withmkstemp(3),mkdtemp(3), and similar calls. For more details, see Using /tmp/ and /var/tmp/ Safely.
I'm not sure how big this particular folder is during downloads though, I see it has .eazstate files after it was finished at least. Might want to make Maxima wipe them after the download is finished regardless, while /tmp should be wiped on reboot (it will be if tmpfs), some people have really long uptime too... So removing them once they're not needed is the correct choice.
But if it's just these tiny files and you don't mind a third function like maxima_temp_dir(), then sure could do that (then we could put them in Windows's %TEMP% aka %APPDATA%\Local\Temp)
maxima-lib/src/util/native.rs
Outdated
| let home = if let Ok(home) = env::var("XDG_CACHE_HOME") { | ||
| home | ||
| } else if let Ok(home) = env::var("HOME") { | ||
| format!("{}/.cache", home) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would keep in mind that cfg(unix) applies to macOS as well, under which ~/Library/Caches would be the right path
not a huge deal as we don't officially support macOS yet, but i'd avoid stacking up future issues for that when the rest of the codebase is fairly friendly on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very fair point actually, but I'm seeing this applies to current maxima_dir() too, where it seems on macOS it's probably supposed to use ~/Library/Application Support instead of ~/.local/share.
In fact, in this case, it seems that our library would apply the exact same logic and outcome on Linux as well:
- https://docs.rs/directories/latest/directories/struct.ProjectDirs.html#method.data_dir
- https://docs.rs/directories/latest/directories/struct.ProjectDirs.html#method.cache_dir
That means the easiest solution will be to just drop the Unix variant and rely on the ProjectDirs. And I guess I'll add some unit tests for that, since I wouldn't feel that comfortable just leaving it like that without some direct documentation what the paths are expected to be.
headassbtw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comment, but LGTM otherwise. sorry for the delay on reviewing, i've been slammed at work
… up with what we had on Linux (includes unit tests to ensure that's the case)
|
No worries, been slammed too. I don't have a setup on Windows, so would appreciate if someone could run the unit test on Windows just to be sure too: No paths should have changed in Windows at all, and on Linux between my first and second commit, but always better safe than sorry. |
So that it doesn't put these files into people's backups.
There were
cache,tempanddownloadssub-dirs previously, all ephemeral. I've put contents of first two directly into~/.cache/maximaand the last one still as sub-dir.I'm only unsure about
download_queue.json, but it feels like it'd be better off also being in cache (if someone wiped cache during a download, it might not necessarily be safe to assume whatever download was going can be just resumed like that, because maybe they just restored their system from a backup after re-setting it up again, maybe game dirs are gone too, etc.)Technically downloader and its queue could go into a state dir, but I think that would be overkill at that point, so let's keep things simple.
As for Windows, data_dir() is
{FOLDERID_RoamingAppData}\_project_path_\data, while cache_dir() is{FOLDERID_LocalAppData}\_project_path_\cache. Well, I guess Origin/EA App do separate their files between these two in a similar fashion, I recall they were putting cached avatars into %LocalAppData% too...