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

desktop: Do not store logs in config directory #17905

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

kjarosh
Copy link
Member

@kjarosh kjarosh commented Sep 14, 2024

Logs are non-essential and can be removed, contrary to config files. This PR changes the log directory from config dir to cache dir.

Platform Old Path New Path
Linux ~/.config/ruffle/*.log ~/.cache/ruffle/log/*.log
macOS ~/Library/Application Support/ruffle/*.log ~/Library/Caches/ruffle/log/*.log
Windows C:\Users\Alice\AppData\Local\ruffle\*.log C:\Users\Alice\AppData\Local\ruffle\log\*.log

Old log files will be migrated to the new path on startup.

@kjarosh kjarosh added the A-desktop Area: Desktop Application label Sep 14, 2024
@kjarosh kjarosh added the waiting-on-review Waiting on review from a Ruffle team member label Sep 14, 2024
@Dinnerbone
Copy link
Contributor

I like it, my only nit is why log and not logs?

@kjarosh
Copy link
Member Author

kjarosh commented Sep 15, 2024

why log and not logs?

I guess the reason is the subconscious desire to stick to conventions used in the Filesystem Hierarchy Standard: /var/log not /var/logs, /usr/include not /usr/includes, /usr/lib not /usr/libs, etc.

@Dinnerbone Dinnerbone added the T-refactor Type: Refactor / Cleanup label Sep 15, 2024
@torokati44
Copy link
Member

torokati44 commented Sep 16, 2024

Some say that logs rather belong to $XDG_STATE_HOME (usually $HOME/.local/state), but it's a Linux-only thing (https://docs.rs/dirs/5.0.1/dirs/fn.state_dir.html), and the consensus seems to be that the second best place for it is indeed the cache directory.
The migration part is a nice touch! Although, maybe if the destination file already exists, it shouldn't be overwritten? 🤔

@kjarosh
Copy link
Member Author

kjarosh commented Sep 16, 2024

Some say that logs rather belong to $XDG_STATE_HOME

I know that there's no obvious place to store log files on Linux (except /var/log for system services). Usually you'd use syslog for the purpose of persisting logs, but I'm not sure about flatpak though.

However, the goal of this PR is not to find the best place to persist logs, but rather to remove them from the obviously wrong place. If they are stored in the config dir, the user has no way of removing them (except manually knowing which files may be removed). That will get exceptionally bad if someone chooses "with timestamp" which will make the config directory grow indefinitely in size.

Although, maybe if the destination file already exists, it shouldn't be overwritten?

I don't think that matters much, logs are migrated before we even try to write to the new directory, and the goal of the migration is not strictly to preserve the logs, but rather to get them out of the config dir.

The file ruffle.log is truncated on start anyway and overwriting logs with timestamps is virtually impossible, unless you're Flash (the other one) and you have an exceptionally mischievous computer.

Logs are non-essential and can be removed, contrary to config files.
This patch changes the log directory from config dir to cache dir.
Ruffle used to save log files in config directory.
This patch makes Ruffle migrate those files at startup.
@torokati44
Copy link
Member

Thank you! ^^

@torokati44 torokati44 merged commit 673f281 into ruffle-rs:master Sep 16, 2024
17 checks passed
@kjarosh kjarosh deleted the log-dir branch September 16, 2024 16:15
@kjarosh kjarosh removed the waiting-on-review Waiting on review from a Ruffle team member label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-desktop Area: Desktop Application T-refactor Type: Refactor / Cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants