stop recording deps in Action_builder.contents

[Alizter]

PR #9552 changed the semantics of Action_builder.contents so that it records a dependency. The Coq rules were relying on the previous behaviour of not recording a dependency in order to generate a build plan. This caused issue #10149 to appear.

This PR fixes #10149 by removing the dependency being recorded in Action_builder.contents matching the previous semantics.

cc @rgrinberg

Here is the semantic change in #9552
https://github.com/ocaml/dune/pull/9552/files#r1573098164

• changelog

[snowleopard]

Wow, that seems like a pretty serious bug. It probably made incremental builds much worse in general, not just for Coq. Thanks for the fix!

Do we have any benchmarks that show an improvement after this commit?

It may be worth patching some earlier releases.

[rgrinberg]

Thanks, could you add a change log entry?

We do not have any incremental benchmarks.

[Alizter]

@rgrinberg I've added a changelog.

I don't see exactly how this would make incremental builds worse outside of Coq. The Coq support appears to be the only serious consumer of Action_builder.contents in terms of using it to generate a build plan. But that also means we have more motivation to keep the original semantics.

I've only mentioned Coq in the changelog since it is the only user-visible effect I am aware of.

[ejgallego]

Thanks for the fix @Alizter , a couple of additional comments:

• I can see how the semantics of contents and dep recording can be confusing some times, maybe it would be worth to make the documentation of contents more precise in the .mli file?
• I have seen various criteria for naming PRs as "refactor", maybe we should clarify the rules of what do we consider as a "refactoring"? [I fear that indeed when we tag stuff as refactor, we tend to review less carefully as most of those are boring / straightforward]