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

[RFC] Add support for order-only and post-use dependencies to Dune engine #10943

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MA0010
Copy link
Contributor

@MA0010 MA0010 commented Sep 20, 2024

This PR (written as part of an internship at LexiFi) is an attempt to add support for order-only and post-use dependencies to the Dune engine. For a discussion of these notions and further background, see #5514. However, for simplicity, we explain the notions briefly:

  • Order-only dependencies: an order-only dependency of a rule is a dependency that is guaranteed to be up-to-date when the rule is refreshed, but that changes to the dependency do not retrigger the rule.

  • Post-use dependencies: a post-use dependency of a rule is an (ordinary) dependency that is declared after the rule action executes. This may sound a bit weird but it becomes interesting when the dependency is also declared as an order-only dependency of the rule.

The prototypical use of these notions is as follows: you have a rule for which the precise set of actual dependencies is hard to specify (eg the set of headers needed to compile a C file), so you declare an overapproximation (eg: all *.h files in a given directory) of the dependency set as order-only dependencies of the rule. When the rule is triggered, it produces as a side-effect the set of precise dependencies (eg gcc -MD) which can then be declared as "actual" (post-use) dependencies. Dune is then able to use this information to avoid retriggering the rule if a non-actual order-only dependency changes.

The PR contains two commits:

First commit

Here, we add support for "order-only" dependencies. This notion is actually already supported by the Dune engine (via the Action_builder.goal combinator), but not currently exposed in the Dune language. This commit exposes this notion in the Dune language via a (order_only <dep>) constructor. There is a small test to exhibit its effect.

Second commit

In this commit we add support for "post-use" dependencies. Concretely we add an action (needed_deps <paths>) that can be used to declare "actual" dependencies as part of the action (the idea being that <paths> are created as a side-effect of previous actions). Each <path> is supposed to contain zero or more S-expressions describing dependencies (following a grammar corresponding almost exactly to a subset of the usual dependency language). A few remarks about the implementation are below. A test for this feature is similarly included.

Both features are documented in the PR, but the intent is not necessarily to expose the features to the user at this time; the documentation was mainly to help understand the features for review. If we want to keep these features, we may decide not to expose them to the user in this way at this time.

What is missing.

  • When one declares "post-use" dependencies in the way described above, it is necessary for correctness that these dependencies be a subset of the declared dependencies of the rule. Otherwise the dependencies in question may not be up-to-date when the action that needs them executes. In other words, the whole point of "post-use" dependencies is that they should already be up-to-date when the action begins to execute and (needed_deps) is only there to help Dune refine the condition for retriggering the rule. This means that (needed_deps) should never actually trigger any building; and it is an error if actual building is necessary. However, this is not checked at this time. I have a patch more or less ready for this, but it seems better to leave this for later once an initial review of the general approach is done.

  • For now, rules using (needed_deps) are not stored in the shared cache. However, in principle this should be possible to do, but it was left for later for simplicity. (Some changes to the semantics of the shared cache appear to be required.)

Indications about the implementation of the second commit

The goal is to use the knowledge of actual/post-use dependencies to optimize rebuilding of a rule. To do that, we reuse the code path used to transmit "dynamic" dependencies (as in the Action plugin) back to the build system. We extend the relevant type to also transmit back the set of needed dependencies gathered during action execution. This set of actual dependencies so obtained are then stored in the local workspace cache entry for the rule. When Dune looks up a rule in the local cache to decide whether to re-execute it (using Rule_cache.Workspace_local.lookup), the digest of the needed deps is checked to decide if the rule is up-to-date or not. If these dependencies are additionally declared as order-only, then the net effect is that the rule will not be retriggered if an order-only, non-actual dependency changes.

The work in this PR was supervised by @nojb.

Assigning to engine expert @snowleopard, but any and all comments warmly welcome.

Signed-off-by: HasanA <mhmd_alameen1023@outlook.com>
Signed-off-by: HasanA <mhmd_alameen1023@outlook.com>
@snowleopard
Copy link
Collaborator

@MA0010 Hi there, thank you for the prototype!

I am about to go on parental leave so I'm afraid I won't be able to review this PR any time soon, but I'd like to say that @rgrinberg and I are planning to upstream our internal changes to the engine in November. Until we do that, reviewing this PR seems hard since the engine has undergone a major rewrite to make it more scalable. We do still have many of the same concepts but I would very much prefer reviewing a patch on top of "actual" code rather than what Dune engine was a year ago (from my perspective).

Perhaps, Rudi can give you some comments while I'm away?

@nojb
Copy link
Collaborator

nojb commented Oct 4, 2024

@rgrinberg and I are planning to upstream our internal changes to the engine in November.

That's good to hear!

I would very much prefer reviewing a patch on top of "actual" code rather than what Dune engine was a year ago (from my perspective).

Makes sense.

I am about to go on parental leave

Congratulations!

@MA0010
Copy link
Contributor Author

MA0010 commented Oct 4, 2024

Hello,

I am about to go on parental leave so I'm afraid I won't be able to review this PR any time soon,

No problem, congratulations!!

We do still have many of the same concepts but I would very much prefer reviewing a patch on top of "actual" code rather than what Dune engine was a year ago (from my perspective).

Yes, makes sense to me. Waiting for the new engine changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants