Skip to content

Use DUNE_CACHE_ROOT instead of XDG_CACHE_HOME in toolchains/revstore caches#12858

Merged
ElectreAAS merged 8 commits intoocaml:mainfrom
ElectreAAS:push-ztpwrqmsuxtk
Dec 15, 2025
Merged

Use DUNE_CACHE_ROOT instead of XDG_CACHE_HOME in toolchains/revstore caches#12858
ElectreAAS merged 8 commits intoocaml:mainfrom
ElectreAAS:push-ztpwrqmsuxtk

Conversation

@ElectreAAS
Copy link
Copy Markdown
Collaborator

@ElectreAAS ElectreAAS commented Dec 3, 2025

This PR is an alternate version of #11612, implementing the 'breaking' option discussed here
It fixes #11584, and helps (but does not fix) #11585.
To recap:

Current behaviour With this PR
DUNE_CACHE_ROOT defaults to XDG/dune/db XDG/dune/
Build cache DUNE_CACHE_ROOT or XDG/dune/db DUNE_CACHE_ROOT/db or XDG/dune/db
Git repo cache  XDG/dune/git-repo only DUNE_CACHE_ROOT/git-repo or XDG/dune/git-repo
Lmdb cache XDG/dune/rev_store only DUNE_CACHE_ROOT/rev_store or XDG/dune/rev_store
Toolchains cache XDG/dune/toolchains only DUNE_CACHE_ROOT/toolchains or XDG/dune/toolchains

@ElectreAAS ElectreAAS mentioned this pull request Dec 3, 2025
Closed
@Alizter Alizter requested review from Alizter and rgrinberg December 3, 2025 15:11
rgrinberg
rgrinberg approved these changes Dec 11, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

The implementation is fine but the name db isn't particularly suggestive of anything. I'd suggest a name like rules or build.

Also, it would be good to have a test that just prints all o these paths with/without configuration.

@ElectreAAS
Copy link
Copy Markdown
Collaborator Author

ElectreAAS commented Dec 12, 2025 •
edited
Loading

The implementation is fine but the name db isn't particularly suggestive of anything. I'd suggest a name like rules or build.

Gotta say I disagree here. Sure the current name isn't the best, but changing that would mean breaking everyone's cache, not just people who have DUNE_CACHE_ROOT set. The previous breakage was deemed acceptable because we assumed not that many people have that variable set (and that's not completely true anyway), but here it almost seems intentional, like we break people's caches just to mess with them.

Also, it would be good to have a test that just prints all o these paths with/without configuration.

Sure!

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Dec 12, 2025

When you say break, you mean we force people to repopulate the cache by rerunning the rules? If so, then this is something that happens regularly.

You can keep the current name if you prefer though. All of this is fairly invisible to users anyway.

@ElectreAAS ElectreAAS force-pushed the push-ztpwrqmsuxtk branch 2 times, most recently from 885dcc4 to 0190e3f Compare December 15, 2025 11:51
@ElectreAAS
Don't use XDG_CACHE_HOME directly, use DUNE_CACHE_ROOT instead
02a32b4 
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS
Expand test to show difference between xdg and dune_cache_dir
24d1526 
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS
Implement 'compromise' option: DUNE_CACHE_HOME
1dc644b 
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS
Move revstore/toolchains cache out of dune_cache_storage and into `…
85a8284 
…dune_util`

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS
Copy link
Copy Markdown
Collaborator Author

ElectreAAS commented Dec 15, 2025

Also, it would be good to have a test that just prints all o these paths with/without configuration.

There is now test/blackbox-tests/test-cases/dune-cache/possible-locations.t

Alizter
Alizter reviewed Dec 15, 2025
View reviewed changes
src/dune_pkg/rev_store.ml Outdated
Comment on lines +157 to +158
(* Why do we create the directory in all cases, and not just when enabled? *)
Path.mkdir_p path;
Copy link
Copy Markdown
Collaborator

@Alizter Alizter Dec 15, 2025

Choose a reason for hiding this comment

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

Let's move that into the enabled branch

@ElectreAAS
Bring in the 'rev_store' cache along into dune_util.
0c4645e 
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS
Implement the 'breaking' option instead of the 'compromise' one.
483472b 
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS
Add small test showing all possible locations of the cache.
493c3d6 
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS
Added changes entry
7fc775d 
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS ElectreAAS merged commit d178296 into ocaml:main Dec 15, 2025
30 checks passed
@ElectreAAS ElectreAAS deleted the push-ztpwrqmsuxtk branch December 15, 2025 16:04
@ElectreAAS
Copy link
Copy Markdown
Collaborator Author

ElectreAAS commented Dec 15, 2025

I'm aware this needs not just a changelog entry (which it has), but a mention in the actual docs. I put off making that section until the back-and-forth between this PR and #11612 was well and truly ended, and so it will land in a subsequent PR.

@ElectreAAS ElectreAAS mentioned this pull request Dec 16, 2025
Merged
shonfeder added a commit that referenced this pull request Dec 19, 2025
@shonfeder
Documentation update to reflect changes made to where caches go. (#12983
d2c85c2 
)

Follow-up to #12858, the documentation needs to be updated to show where
things go.

Draft mode because I'm not the best technical writer

Rendered output visible here
https://dune--12983.org.readthedocs.build/en/12983/reference/caches.html
jonludlam pushed a commit to jonludlam/dune that referenced this pull request Jan 14, 2026
@ElectreAAS @jonludlam
Use DUNE_CACHE_ROOT instead of XDG_CACHE_HOME in toolchains/revstore …
43aa850 
…caches (ocaml#12858)

This PR is an alternate version of ocaml#11612, implementing the 'breaking'
option discussed
[here](ocaml#11612 (comment))
It fixes ocaml#11584, and helps (but does not fix) ocaml#11585.

---------

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@Alizter Alizter Alizter approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Toolchains cache does not follow DUNE_CACHE_ROOT

3 participants

@ElectreAAS @rgrinberg @Alizter