Extend `enabled_if` for alias stanza to variable available in actions Fixes #3832

[bobot]

This MR allows to use variable like %{lib-available:mylib} in the enabled_if of alias stanza. It is possible because alias stanza don't have any targets. However it was harder to do because the expansion of an action or dependencies are a Build.t. So it depends on #3963.

It is possible to simplify this MR since the action field is incompatible with extended enable_if by always computing the dependencies of the alias, and by using Build.dyn_deps.

[snowleopard]

This is still marked as Draft, so I'm unsure if it's ready for review. Please ping me if/when it is!

[bobot]

It is rebased on top of #3963, so that this MR just add a use of a staged Build.t. It will be ready if #3963 is merged.

[bobot]

ping @snowleopard it is ready for review.

[snowleopard]

@bobot Could you rebase this please? I can see some changes from #3963, and some changes appear to be conflicting (e.g. renaming of Catch which had been undone in #3963).

[bobot]

Indeed; rebased.

[bobot]

I need to add a guard for dune 2.9

[bobot]

Ok so there are things that I just understood now

There are three possibilities for variables:

• they can be known without reading any dune file
• they are known without executing any external command (e.g. %{lib-available: })
• they are dynamic (e.g. %{read: })

Only the last requires the val Action_builder.Expert.action_builder: 'a t t -> 'a t.

The action_builder was buggy because the action_deps and rule_deps should be built.

[snowleopard]

@bobot I find it difficult to fully understand the general design of enabled_if. The new complexity that this PR introduces doesn't seem great.

As a very basic design question, what should happen in this case?

(library
 (name a)
 (enabled_if %{lib-available:b}))

(library
 (name b)
 (enabled_if %{lib-available:a}))

And what about this?

(rule
 (target false.txt)
 (action (write-file false.txt "false")))

(alias
 (name foo)
 (enabled_if {%read:false.txt}))

The alias should be disabled, but if it's disabled, the rule shouldn't fire when you execute the alias, but I guess it will.

[bobot]

For library case, this MR should not change anything to the situation. Only variables of the first kind (Cf. my comment about the three kind of variables) is allowed in enabled_if of library stanza.

For the second case, the semantic is indeed not "if an alias is disabled then it is as if it is not present". The dependencies of the enabled_if will always be built.

[snowleopard]

For library case, this MR should not change anything to the situation.

Agreed. I mentioned this example just to illustrate my general concerns about enabled_if fields -- they seem to be difficult to reason about and can give rise to various non-monotonic behaviour.

The dependencies of the enabled_if will always be built.

Right, this makes sense. Let's add this example as a test, perhaps also adding a couple of echos to the rule and to the alias, to make it obvious that the rule is executed but the alias is not.

Also, we need to make sure our cycle detection logic doesn't fall apart on cases like this:

(rule
 (target false.txt)
 (deps (alias foo))
 (action (write-file false.txt "false")))

(alias
 (name foo)
 (enabled_if {%read:false.txt}))

(At least I assume this is going to be a cycle...)

[bobot]

The output with the dependency cycle is quite strange:

 |# Try to create a dependency cycle
 |
 |  $ cat >dune <<EOF
 |  > (rule
 |  >  (target false.txt)
 |  >  (deps (alias foo))
 |  >  (action (write-file false.txt "false")))
 |  >
 |  > (alias
 |  >  (name foo)
 |  >  (enabled_if {%read:false.txt}))
 |  > EOF
 |
 |  $ dune build @foo
+|  File "dune", line 8, characters 13-30:
+|  8 |  (enabled_if {%read:false.txt}))
+|                   ^^^^^^^^^^^^^^^^^
+|  Error: This value must be either true or false got:
+|  "{%read:false.txt}"

EDIT: Quiz: there is an error in this test can you find it!

[snowleopard]

The output with the dependency cycle is quite strange

Indeed, that looks like a bug.

[Kakadu]

@bobot It looks like that PR is temporarily abandoned... Any plans to resurrect it for next or next+1 release?

[bobot]

I didn't get time lately, in addition to vacation for developing in dune. Moreover this problem seems not directly linked to this issue. So I will rebase this work, check if the problem is still present after the fixes about cycles done in other MR. And if the problem is still present open an issue about it. @snowleopard do you think it is not inacceptable?

[snowleopard]

I didn't get time lately, in addition to vacation for developing in dune. Moreover this problem seems not directly linked to this issue. So I will rebase this work, check if the problem is still present after the fixes about cycles done in other MR. And if the problem is still present open an issue about it. @snowleopard do you think it is not inacceptable?

@bobot The semantics of enabled_if is already complex, and this PR adds further complications. As the test demonstrates, the current implementation is clearly incorrect: perhaps, because of some existing issues, perhaps because of new ones.

I'm hesitant to add more non-trivial code to the part of Dune that we already don't understand well. My preference would be to get back to the drawing board, come up with a good overall design for enabled_if and then add an implementation that conforms to that design.

[bobot]

So it took me time to rebase above the simplification of the expansion; I nearly restarted from scratch.

Some remarks:

• The expansion mechanism is indeed simpler (but @jeremiedimino can perhaps check what I done).
• However %{lib-available:...} is already accepted in main since those simplifications, even with < 2.9 @jeremiedimino . I previously added a check for that in this branch but it is now seemingly a noop.
• The cycle still create a strange error message.
• @snowleopard We can discuss the semantics of the enable_if if there are not clear, what are your concerns? The main point for me is to define precisely the three kind of variables and clarify in the documentation and the code the enable_if of which stanza can access which.

[snowleopard]

@snowleopard We can discuss the semantics of the enable_if if there are not clear, what are your concerns?

Happy to discuss the semantics. My current concern is that the only sensible semantics of my cyclic example seems to be a dependency cycle error, but the current implementation does something else.

So, if I had a semantics written down somewhere, I could point to it and say: look, the current PR doesn't implement the design. But there is no written semantics, so I can't :-)

[bobot]

My current concern is that the only sensible semantics of my cyclic example seems to be a dependency cycle error, but the current implementation does something else.

I completely agree with your semantic. ~~And I agree this PR doesn't follow the semantic.~~ :sob: :sob: the test for cycles was wrong {%read instead of %{read. With the right test, we can see that the cycle is correctly handled because it is independent of the PR.

[snowleopard]

the test for cycles was wrong {%read instead of %{read. With the right test, we can see that the cycle is correctly handled because it is independent of the PR.

Oh, sorry, it was my fault. But it's great that the test now passes!

Looks like the implementation is almost new, so I'll have to re-review it.

[bobot]

Looks like the implementation is almost new, so I'll have to re-review it.

Yes the modifications in main were quite big. I'm afraid that we lost the ability to cherry-pick this feature to 2.9 perhaps the old version will be cherry-pickable, but it start to be complicated. Do you have an opinion @rgrinberg ?

[ghost]

I haven't followed the whole discussion, happy to jump in a whereby to discuss. But regarding the evaluation, after https://github.com/ocaml/dune/pull/4314 we will be able to have something much more principled, without all these horrible static approximations. I'd like to kill all the remaining ones and get rid of Action_builder.static_eval and Expander.Static. So please don't add more of these :)

[bobot]

Thanks that's good to know. I can remove them, it was only to have that %{unknown} && false be directly false instead of building and waiting for %{unknown}. But we could advice the users to add the simple to evaluate formula first.

Can we already code with the Action_builder API something like this?

val reduce:
 'a t -> 'b t
 -> left:('a -> 'c option)
 -> right:('b -> 'c option)
 -> both:('a -> 'b -> 'c)
 -> 'c t
(** [reduce l r ~left ~right ~otherwise] is a computation which try to compute the result
     before [l] and [r] are both available. [left] (resp. [right]) is used if [l] (resp. [left])
     is already available. If the function return [None], the function wait for [r] (resp. [l])
     and execute [both]. It is possible that only [both] is executed. *)

[ghost]

Well it's a monad now, so we can do whatever we want.

Thanks that's good to know. I can remove them, it was only to have that %{unknown} && false be directly false instead of building and waiting for %{unknown}. But we could advice the users to add the simple to evaluate formula first.

I suggest to wait for my PR to be merged, and then you'll be able to make the blang evaluation be in the Action_builder monad. Which will make everything simpler and without weird limitations.

[ghost]

Hmm, that said, you might also need Lib to be in the memo monad, which is another story. But if we aim for 3.0, then that's fine. We'll need to tackle lib before 3.0 anyway.

[nojb]

What is the status of this?

[rgrinberg]

This is already available in 3.0