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

Compiler cache: honor XDG_CACHE_HOME on MacOS #716

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

matthiasdiener
Copy link
Contributor

@matthiasdiener matthiasdiener commented Mar 19, 2024

Similar to inducer/pytools#204

@matthiasdiener matthiasdiener force-pushed the xdg_cache_home_macos branch 2 times, most recently from 69e5e2d to 82519f1 Compare March 19, 2024 21:07
@alexfikl
Copy link
Contributor

I understand the need here, but I'm a curious about some things:

  • Why only on macOS and not Windows? Can the Windows paths be overwritten by environment variables?
  • If we're forcing a non-native environment variable on other OSes, why not use something like PYOPENCL_CACHE_DIR (to mirror PYOPENCL_NO_CACHE)?

@matthiasdiener
Copy link
Contributor Author

matthiasdiener commented Mar 19, 2024

  • Why only on macOS and not Windows? Can the Windows paths be overwritten by environment variables?

Looking at https://github.com/platformdirs/platformdirs/blob/main/src/platformdirs/windows.py#L251, I guess so, but my knowledge of Windows is almost zero 😞

  • If we're forcing a non-native environment variable on other OSes, why not use something like PYOPENCL_CACHE_DIR (to mirror PYOPENCL_NO_CACHE)?

We could add PYOPENCL_CACHE_DIR, but the current goal was to make the disk caching selection consistent across loopy and pyopencl, for both Mac and Linux.

Edit: another important reason: Pocl also honors XDG_CACHE_HOME on MacOS.

@alexfikl
Copy link
Contributor

alexfikl commented Mar 20, 2024

Edit: another important reason: Pocl also honors XDG_CACHE_HOME on MacOS.

Fair enough, but they just fallback to it and also have a POCL_CACHE_DIR (at least that's what the docs say).

@matthiasdiener
Copy link
Contributor Author

This is ready for review @inducer.

@inducer inducer merged commit 9ad9b00 into inducer:main Mar 22, 2024
17 checks passed
@inducer
Copy link
Owner

inducer commented Mar 22, 2024

LGTM, thanks!

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.

3 participants