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

Use platform-specific cache paths and respect XDG_CACHE_HOME #3458

Merged

Conversation

liby
Copy link
Contributor

@liby liby commented Dec 27, 2024

Description:

Use proper platform-specific methods to determine cache directory paths:

  1. First check XDG_CACHE_HOME environment variable

  2. On macOS:

    • Use NSFileManager.URLForDirectory to get system cache path (~/Library/Caches)

    • Use bundle ID (com.mitchellh.ghostty) as base directory to follow macOS conventions

  3. Fall back to XDG spec defaults for other platforms (~/.cache)

Changes:

  • Extract common NSFileManager path lookup logic into makeCommonPath helper

  • Use NSFileManager.URLForDirectory to get proper macOS cache directory

  • Use bundle ID for macOS cache directory to match system conventions and appSupportDir behavior

  • Add tests to verify path construction for different platforms

Questions for reviewers:

  1. Should we add migration logic for existing cache files?

  2. Or should we document this as a breaking change and let users manually clean up?

Testing:

  1. Added unit tests for path construction:

    • macOS: verifies ~/Library/Caches/com.mitchellh.ghostty path

    • Linux: verifies ~/.cache/ghostty path (XDG spec)

  2. Verified tests pass with zig build test

Fixes #3202

src/os/xdg.zig Outdated Show resolved Hide resolved
src/os/xdg.zig Outdated Show resolved Hide resolved
@liby liby changed the title Use $HOME/Library/Caches on macOS instead of $HOME/.cache Use platform-specific cache paths and respect XDG_CACHE_HOME Dec 28, 2024
@liby liby force-pushed the bugfix/fix-crash-when-home-directory branch from 2755df7 to 074ff33 Compare December 28, 2024 00:31
@liby liby requested review from tristan957, vahur and jparise December 28, 2024 00:33
src/os/xdg.zig Outdated Show resolved Hide resolved
src/os/macos.zig Outdated Show resolved Hide resolved
src/os/macos.zig Show resolved Hide resolved
src/os/xdg.zig Outdated Show resolved Hide resolved
@liby liby force-pushed the bugfix/fix-crash-when-home-directory branch from 074ff33 to 3b5bc77 Compare December 28, 2024 00:49
@liby liby requested a review from jparise December 28, 2024 00:50
Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

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

I think this is generally a good change and addresses a blocker for some people on macOS, so I'm motivated to see a solution get merged. Thanks for working on this!

There are some follow-ups that I think are interesting to think about afterward, but I don't think any of them are blocking.

src/os/macos.zig Outdated Show resolved Hide resolved
src/os/xdg.zig Outdated Show resolved Hide resolved
Copy link

@vahur vahur left a comment

Choose a reason for hiding this comment

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

Not really able to review the changes as I don't know the codebase. As long ghostty starts with read-only home directory and uses platform directories, it's enough for me atm. Parity with Application Support, bundle id wise, would be nice but not a show stopper.

@liby liby force-pushed the bugfix/fix-crash-when-home-directory branch from 3b5bc77 to 99e8604 Compare December 28, 2024 09:13
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

I don't think xdg.cache should return anything but the XDG cache dir as specified.

I think the calling point for xdg.cache should be modified to call this for macOS instead.

@liby liby force-pushed the bugfix/fix-crash-when-home-directory branch from 99e8604 to 39a0ef3 Compare December 30, 2024 01:43
@liby liby requested a review from mitchellh December 30, 2024 01:43
@liby liby force-pushed the bugfix/fix-crash-when-home-directory branch from 39a0ef3 to 10bbc26 Compare December 30, 2024 01:45
src/crash/sentry.zig Outdated Show resolved Hide resolved
@liby liby force-pushed the bugfix/fix-crash-when-home-directory branch 2 times, most recently from c7dcf2a to 73eb21a Compare December 31, 2024 01:14
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. I noted one issue/question.

src/os/xdg.zig Outdated Show resolved Hide resolved
@liby liby force-pushed the bugfix/fix-crash-when-home-directory branch from 73eb21a to a605c74 Compare January 2, 2025 16:22
@liby liby requested a review from mitchellh January 2, 2025 16:23
@liby liby force-pushed the bugfix/fix-crash-when-home-directory branch from a605c74 to 6fca269 Compare January 2, 2025 16:24
@mitchellh mitchellh enabled auto-merge January 2, 2025 19:51
@mitchellh mitchellh merged commit afdaaf1 into ghostty-org:main Jan 2, 2025
21 checks passed
@github-actions github-actions bot added this to the 1.0.2 milestone Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ghostty crashes if home directory or $HOME/.cache is not writable
6 participants