Before this default is flipped, we need to have a story for how this interact with user rules. As a first approximation, all user rules are unsafe to cache this way. So users should be able to opt-in to enable caching for their own rules.

FMU, as this PR is only changing the default, if the user has a configuration, it will take it instead of the default. Or am I missing something? 😕

The problem lies in that most users do not have such a configuration. Moreover, most project contain user rules of the form:

(rule
 ..)

That are absolutely not safe to cache. Therefore, flipping the default like is done in this PR is highly likely to cause a lot of unintended breakage.

Incorporated (very helpful) comments by Rudi on #10789, we now have a different environment variable named (for now) DUNE_CACHE_USER_RULES defaulting to false, the old DUNE_CACHE is now on by default, and Action.Full.t are now uncacheable by default.
What remains to be done is picking which of the 36 uses of Action.Full.make can be safely cached, and updating failing tests.

The intended behaviour is now, as far as I understand correctly (Writing mostly for my future self here) :
By default the cache is now available, however most stuff won't go into it
The only things that do go into it are the uses 'blessed' by that can_go_in_shared_cache parameter (looking at package rules obviously, but maybe more later?)
What the users can do:

• disable the cache if they don't want the new default (is that considered a breaking change?)
• enable caching anything not 'blessed' by using the new env variable

disable the cache if they don't want the new default (is that considered a breaking change?)

I don't think so. Since disabling the cache will only affect the package management rules which aren't stable.

disabling the cache will only affect the package management rules which aren't stable.

I think I am misunderstanding something because as we see in the tests, a lot of them fail if DUNE_CACHE_USER_RULES=enabled is not set, so to get the previous behavior of DUNE_CACHE=enabled you also need DUNE_CACHE_USER_RULES=enabled. This seems like a breaking change?

That are absolutely not safe to cache.

@rgrinberg can you remind me why user rules are not safe to cache?

That are absolutely not safe to cache.

@rgrinberg can you remind me why user rules are not safe to cache?

From experience, I can say that many rules do not specify their dependencies correctly. Some rules aren't even deterministic.

These kinds of rules can poison the cache without an easy way to diagnose the issues.

I don't think the Nix failures have anything to do with the PR. I'll add the overrides for rules from package management in a second PR so this is just laying down the groundwork for it.

As mentioned on slack, the error is relevant. Dune is trying to create the cache directory at $HOME and nix is rightfully forbidding that.

One way to work around this would be to disable the cache if the cache dir isn't writeable.

Maybe just emit a warning?

@rgrinberg I've added some code that detects if the directories can't be created, it disables the cache and warns.

I had to add a case to mkdir_p_result which has the downside that mkdir_p_result now has success and error cases (mirroring mkdir_result). I think that's not super nice, as it probably would be better to split the cases into an "there's a directory" (Ok) and "couldn't be created" (Error) category but on the other hand I didn't want to overengineer this.

Why is it a big regression? You set an environment variable and move on.

Setting env vars across different build setups can be pretty time-consuming and often not trivial.

I agree with @samoht that this is a regression for users.

I also agree that this is a regression. I hope "set some new environment variable and move on" is not a typical experience when upgrading to a new version of Dune. We should find a better way!

We tried to find a better way but this is all we could come up with.

The cache has to be enabled for package management, but doing so while opting in to cache all user rules would be even more disruptive. At least this way the disruption is limited to the small subset of users using the cache.

Suggestions are welcome.

Well, package management is a new feature, and it seems fair to require early adopters to, say, set DUNE_CACHE_RULES=false. And tell them to bug the rule authors of rules that require that.

In general, penalising rules that do the right thing seems surprising. Surely, the incentives should go the other way around.

Well, package management is a new feature, and it seems fair to require early adopters to, say, set DUNE_CACHE_RULES=false. And tell them to bug the rule authors of rules that require that.

This suggestion is with conjunction to enabling the dune cache by default?

It might be worth recalling why we never enabled the cache by default in the first place. Our own OCaml rules had some correctness issues we never got to the bottom of (around C compilation and linking IIRC). User rules were in an even worse spot. Those were mostly unsandboxed and thus often broken. I don't think the situation has changed much since.

I don't want to punish users that write correct rules, but I also know it would be way too optimistic to assume such rules are correct by default either.

People who enabled the cache are a minority of advanced users, so I think putting the burden of maintaining config on them is more fair than on the users of package management. Some of those are going to be beginners that are just getting started with dune.

People who enabled the cache are a minority of advanced users, so I think putting the burden of maintaining config on them is more fair than on the users of package management.

I've been using dune-cache extensively for years now. The subset of the ecosystem I am working with does not seem to have any broken rules. So I'm surprised that you are so conservative here (even if I understand the rationale, I'm not sure I agree with the proposed workaround). In any case, I would much rather edit ~/.config/dune/config once and for all with (dune_super_cache enabled) than remember to set up an env variable in my shell :-)

Another thing to consider is what will happen to ocaml github action.

IIANM it uses the cache, so it would require adaptation?

Indeed there is large use of dune cache (and it is often why Coq people migrate from make to dune, to have a cache), so IMHO the user experience of this change could be not super good, if that silently stops working.

There is now a changelog entry proposed tho.

Maybe a good compromise would be to lead towards a setup more friendly to the "status quo" of users, we could have a setup such that:

• (cache enabled): old behavior, all rules are cached
• (cache safe): new behavior, only cache "safe" rules

Dune package management could emit a warning if users are running with (cache enabled).

WDYT folks?

I appreciate all the discussion going on here, hearing differing viewpoints is always nice.

I think I am convinced by your arguments @ejgallego & @samoht.

My idea of what to do next would be almost the same as what was proposed above :

1. Any tinkering should be doable both by env vars and by config file editing
2. Make 3 modes for the cache: completely disabled (the old default), completely enabled, and the new "we only cache things we are 100% sure are cacheable for all cases".
3. We move the default to the safe mode, while not requiring users change their existing config. That way it wouldn't break anyone's workflow

Sounds great to me @ElectreAAS ; maybe we could actually deprecate (cache enabled) too, in favor of something like (cache unsafe) ?

This can be done properly using (dune lang ...) I think, modulo layer issues (I hope not)

I like the idea of using (dune lang ...) in config to control this kind of behavior, however it may not be always doable.

Another thing to consider is what will happen to ocaml github action.

IIANM it uses the cache, so it would require adaptation?

Indeed there is large use of dune cache (and it is often why Coq people migrate from make to dune, to have a cache), so IMHO the user experience of this change could be not super good, if that silently stops working.

There is now a changelog entry proposed tho.

Maybe a good compromise would be to lead towards a setup more friendly to the "status quo" of users, we could have a setup such that:

• (cache enabled): old behavior, all rules are cached
• (cache safe): new behavior, only cache "safe" rules

Dune package management could emit a warning if users are running with (cache enabled).

WDYT folks?

I'm lukewarm about it. I think having the cache enabled or disabled vs selecting which rules are cacheable are orthogonal things and mixing them up in a single config will lead to confusion and lost flexibility down the road. On the other hand, if it really is so difficult to move to a different way to enable the cache for the coq devs (how did you get them on all on board to use the cache in the first place?), then so be it.

I'm lukewarm about it. I think having the cache enabled or disabled vs selecting which rules are cacheable are orthogonal things and mixing them up in a single config will lead to confusion and lost flexibility down the road.

That is a very interesting observation. Indeed I assumed the opposite, that enabling the cache and understanding what gets cached were very related concepts.

My thought process is like this: imagine I do (cache enabled), but then no rule gets cached because all my rules are unsafe. That seems pretty confusing to me.

We could indeed split the config in something like (cache enabled/disabled) and (cache_config safe/full), however I am not sure how this is better?

What would an example of "confusion" or "lost flexibility" in the scenario look like?

On the other hand, if it really is so difficult to move to a different way to enable the cache for the coq devs (how did you get them on all on board to use the cache in the first place?), then so be it.

I am not so worried about the Coq devs, that's a small set of users, but more about a lot of users (mainly "industrial") that rely on the cache. In general, this is a de facto breaking change and may require significant fiddling with things like the github ocaml action etc...

how did you get them on all on board to use the cache in the first place?

I am not sure I understand the question, I guess once they saw it in action most enabled it, a full Coq compilation is very expensive, so caching pays off immensely.

Why not proceed as follows:

• DUNE_CACHE=enabled -> cache is fully enabled (like before this PR)
• DUNE_CACHE=partial -> cache is enabled but not for user-defined rules (like in this PR)
• DUNE_CACHE=disabled -> cache is fully disabled (like before this PR)

Then this PR should amount to changing the default to DUNE_CACHE=partial. This is completely backwards compatible (meaning that people who have already set DUNE_CACHE do not observe any difference in behaviour), while enabling the Dune cache by default, as in this PR.

but then no rule gets cached because all my rules are unsafe. That seems pretty confusing to me.

That's the situation today, but I think we can start marking some rules as safe to cache gradually. I'm thinking ocamldep rules, user rules that are sandboxed, some of the compilation rules.

What would an example of "confusion" or "lost flexibility" in the scenario look like?

Here's the main difference: when you mark a rule as unsafe for caching, this bit ends up being included in the action digest. The consequence of doing this is that once this bit is flipped (via the environment variable for example), the action will re-run and the cache entry will be populated (or possibly trimmed if going the other way). On the other hand, enabling/disabling the cache has no effect on the rules themselves. So it can never trigger recompilation.

Maybe this distinction isn't particularly important or the two types of settings can be unified somehow in the future?

As I said, we can go along with your suggestion if this will make transitioning easier for you. It's good enough for us to proceed on the package management side.

I am not sure I understand the question, I guess once they saw it in action most enabled it, a full Coq compilation is very expensive, so caching pays off immensely.

My point is that you somehow told the team that a config file needs to be updated to enable the cache. Well, you can tell them that a new config is needed in the same way. Or, if you have some makefile you all share, you can just set the config there.

@rgrinberg what do you think of the concrete proposal above ? It would provide the same effect as in this PR while remaining fully backwards compatible, as far as I can see.

It sounds quite similar to Emilio's proposal. Seems fine as well.

It sounds quite similar to Emilio's proposal. Seems fine as well.

See #10944

My point is that you somehow told the team that a config file needs to be updated to enable the cache. Well, you can tell them that a new config is needed in the same way. Or, if you have some makefile you all share, you can just set the config there.

Indeed, this is why I was requesting for some setting in .config, vs an env var.

In the Coq world there a multitude of different CI / testing deployements, and not only Coq devs, but many other users would be affected by this change.

Unfortunately there is nothing close to a homogeneous Coq build CI setup (we are trying to improve that, check your mail if you are interested ;)), but indeed I don't know how to write a guide on adding a new env var effectively, there are just too many combinations of shells and dev setups, it seems to me. The config file is nicer to implement I feel.

A bit orthogonal to this is the topic of backwards compat, IMO dune has been doing superb on this aspect, so changes like this are better controlled by (dune lang ...) version so users can control breakage better.

I am often bitten by rules that are cached wrongly, so indeed it is great to progress on this front, so we get better tools to debug and mark bad rules.

Another thing that you might find worth doing is to mark all the coq rules as being explicitly safe. If you're confident about those, might as well enable them under enabled-except-user-rules as well.

Another thing that you might find worth doing is to mark all the coq rules as being explicitly safe. If you're confident about those, might as well enable them under enabled-except-user-rules as well.

Indeed thanks! I've added this to the list of TODOs to the (coq lang 1.0) effort.

One reason we didn't mark them as safe, is that while Coq rules are "cache safe", they are not "sandbox" safe, due to the "hack" of including hashes of deps in .vo files.

We tried sandboxing the Coq rules, and they were way slower, on the other hand we'd love to have a flag to do that so we can do "sandbox but slow" and "not sandbox but fast". Of course, it'd be even better to make sandboxing faster, but in this case it is a bit hard due to the number of egdes added to the build graph in Coq sandboxed mode.

This is added to the working group TODO, will come back to the topic when we discuss what to do.