[RFC] replace action field of alias stanza by alias field in rule stanza

[ghost]

Right now, when one writes:

(alias
 (name blah)
 (deps x)
 (action (run foo)))

It's not entirely clear whether x is a dependency of the action or the alias. In particular, Dune and Jenga (inside Jane Street) accidentally made two different choices which indicates that the semantic is not obvious. In Jenga, x is a dependency of the action but not the alias, and in Dune x is a dependency of both.

Additionally, it always a bit annoying when the action produces something as aliases are not supposed to have targets.

To clarify all this, I propose the following change in the 2.x version of the language:

• we remove the action field from the alias stanza
• we add an alias field to the rule stanza

adding (alias foo) to a rule would basically add the targets as dependency of the alias, even if the rule has no targets.

[aalekseyev]

First, I want to say that "dependency of the alias" sounds ambiguous to me because alias has two kinds of dependencies and we should try to clarify that terminology.

Consider a rule like this:

(rule (target x) (dep (alias foo))) (action ...))

Let's call the set of files whose changes cause the rebuild of "x" an "expansion" of the alias foo. (similar to expansion of a variable, as @rgrinberg suggested)

Where @diml mentions dependencies of the alias above he clearly means the expansion of the alias.

The second idea of dependencies is the set of actions that contribute to the build result (transitively or directly) when you ask dune to build an alias. I like to call those "dependencies", but for clarity here let's call them "transitive dependencies" (for avoidance of doubt, transitive deps are not obtained by transitively closing over expansions).

When the rule is assigned to the alias it should absolutely become the "transitive dependency" of the alias, but I'm much less convinced that its targets should become a part of alias "expansion". In fact I would expect the rule to contribute nothing at all to the alias expansion.

In the meeting we also discussed that the idea of alias expansions should be eventually deprecated in favor of variables. I like that idea and if we're moving that way I think it makes sense to avoid adding more things to the expansions.

Other than that, I very much support the idea and on top of simplifying aliases (which is a win already) it makes a common idiom shorter.

Instead of writing

(rule
  (targets foo)
  ...
)
(alias (name DEFAULT) (deps foo))

you just write:

(rule
  (targets foo)
  (alias DEFAULT)
  ...
)

One implementation detail that could be annoying is to add support for rules with an empty set of targets. Maybe it's not too difficult though.

[ghost]

I thought more about this and I agree that we shouldn't add the targets to the expansion of the alias. So happy to drop that part.

Regarding rules with an empty set of targets, we can do something similar to what we do now for aliases with actions, i.e. replace <action> by (progn <action> (create-file <stamp-file>)) and add the stamp file as a transitive dependency of the alias. We can do that whether the rules has targets or not.

[aalekseyev]

That sounds like it should work.

[nojb]

This looks like a good idea. A small thing that would be useful as well is to make sure that rules with an (alias ...) field are not attached to the all alias by default. Currently all user-defined rules are attached to all. This is inconvenient when one uses the common idiom mentioned by @aalekseyev above to build a testsuite, as it results in the output-producing part of the testsuite running when you do dune build.

[nojb]

Regarding rules with an empty set of targets,

Just to understand better, can you show me an example of such a rule?

[aalekseyev]

can you show me an example of such a rule [with an empty set of targets]

Such rules are not currently supported. The idea is that they will replace the action field of aliases. For example where we currently write:

(alias
 (name runtest)
 (action
  (run ./some_tests.exe)))

We would instead write:

(rule
 (alias runtest)
 (action
  (run ./some_tests.exe)))

[aalekseyev]

I'm not particularly excited about the idea of removing things from the @all alias. It does say "all" in its name, after all.

[nojb]

I'm not particularly excited about the idea of removing things from the @all alias. It does say "all" in its name, after all.

My issue is not with the all alias but with dune build which should not in my view trigger all user-defined rules. The case I have in mind when you have a generated testsuite that may consist of several hundreds of rules producing test output, and triggering them dune build is a bit unexpected.

[nojb]

(but this is more or less orthogonal to the issue at hand, so let's discuss this elsewhere)

[rgrinberg]

While working on this I found that we the current alias stanza with action is slightly more flexible than this proposal. It allows for a package field to be set to allow one to attach aliases to packages (particularly useful for tests). I think we need a similar field for rules.

[ghost]

Removed from the 2.0.0 milestone, it feels like a too big change at this point.

[rgrinberg]

Ah well it’s essentially done at this point. Anyway, I won’t press as I can’t see a reason why it can’t ship in 2.1

…

On Oct 28, 2019, 10:08 PM +0900, Jérémie Dimino ***@***.***>, wrote: Removed from the 2.0.0 milestone, it feels like a too big change at this point. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

[rgrinberg]

I still need the 2nd part of this issue: deprecate the action field for the alias stanza. Do we need to add an intermediate deprecation message somewhere? Or is it sufficient that we simply disallow action in for >= 2.0?

[ghost]

Whether we disallow or deprecate the action field, the replacement needs to be fully ready. In particular, we need the package field for rules.

If we do all this for 2.0.0, then we can effectively disallow action for >= 2.0. Otherwise, it needs to be only deprecated and we'll get rid of the action field in alias in 3.0.

I'm just a bit worried that this looks like a big change close to release time. But apart from that I have no objection.

[rgrinberg]

Whether we disallow or deprecate the action field, the replacement needs to be fully ready. In particular, we need the package field for rules.

I must have forgot mention this in the PR but I ended up doing this already. The rule stanza covers all the use cases in master. I can port the alias uses with action in dune itself to show this

…

On Nov 4, 2019, 8:12 PM +0900, Jérémie Dimino ***@***.***>, wrote: Whether we disallow or deprecate the action field, the replacement needs to be fully ready. In particular, we need the package field for rules. If we do all this for 2.0.0, then we can effectively disallow action for >= 2.0. Otherwise, it needs to be only deprecated and we'll get rid of the action field in alias in 3.0. I'm just a bit worried that this looks like a big change close to release time. But apart from that I have no objection. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

[ghost]

Ok, no objection to doing the switch now and disallowing the action field in >= 2.0 then!

[rgrinberg]

See this pr: #2846

[ghost]

This is now complete