Skip to content

Conversation

@ElectreAAS
Copy link
Collaborator

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

Copy link
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
Collaborator Author

ElectreAAS commented Dec 12, 2025

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
Member

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
Copy link
Collaborator Author

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

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
Collaborator

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 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
Collaborator Author

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.

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.

Toolchains cache does not follow DUNE_CACHE_ROOT

3 participants