Add when action available in lockfiles

[gridbugs]

The when action will be used in package management to implement build commands which are conditional on some filter (this is a feature of opam we need to copy). The nothing action does nothing and is used internally to handle the case when when's condition is false. The nothing action is exposed as it can be handy when writing dune files to be able to temporarily create an action which does nothing when you're not sure which action to use but you need the file to parse correctly to test some other stanza (currently it's possible to get the same effect by using (progn) with no arguments but this is a hack).

[Alizter]

Why not expand them to (progn)? That way you don't need to modify action_exec.

[gridbugs]

Why not expand them to (progn)? That way you don't need to modify action_exec.

I thought about doing that but it felt like a hack and that it would be clearer to make it a separate type of action_exec. Happy to change it if that's the consensus though.

[Alizter]

There is obviously some overlap with

• feature: case and cond statements #8324

I think we will need to choose one of them as otherwise we will have too many ways of doing the same thing.

[Alizter]

At first I thought this could be a simpler replacement for case and cond, but upon further inspection we lose the ability to stop at the first branch. Also another way to write this would be to use concurrent (on the last two args anyway). That way you wouldn't have to sequentially do all the checks.

[gridbugs]

It's deliberately much simpler than case and cond and it's supposed to behave like the when keyword in many lisp languages. In practice it's going to be used to filter out opam commands. It might not be practically useful to expose this in non-pkg files, at least for now. Possibly this example is misleading as it makes it look like a case statement when it isn't one. @rgrinberg what do you think?

[gridbugs]

Also another way to write this would be to use concurrent (on the last two args anyway). That way you wouldn't have to sequentially do all the checks.

Do you just mean as an optimization?

[rgrinberg]

Possibly this example is misleading as it makes it look like a case statement when it isn't one. @rgrinberg what do you think?

I'd rather introduce whatever is needed to keep lock files as small and as readable as possible.

[gridbugs]

To clarify I was asking whether it makes sense for when to be available in dune files or only in lock files.

[rgrinberg]

At the moment? There's no need to make it available. Eventually, yes the plan is to completely unify dune and lock files.

[gridbugs]

There is obviously some overlap with

* [feature: case and cond statements #8324](https://github.com/ocaml/dune/pull/8324)

I think we will need to choose one of them as otherwise we will have too many ways of doing the same thing.

I don't think a when action necessarily precludes case or cond. Plenty of lisps effectively have when, if/else (effectively cond), and case statements.

[rgrinberg]

I thought about doing that but it felt like a hack and that it would be clearer to make it a separate type of action_exec. Happy to change it if that's the consensus though.

It would be better to avoid changing it.

[rgrinberg]

Note that the nothing action should be (nothing). All actions are forms.

[gridbugs]

Note that the nothing action should be (nothing). All actions are forms.

In that case I'd find it clearer to not add nothing and instead use (progn) to represent a no-op (both internally and in dune files). Though I think we have to be careful to prioritize developer experience over staying true to lisp. The "All actions are forms." rule may make sense from a lisp perspective but as a user who's barely used lisp I'm used to parentheses only being necessary when something takes a parameter.

E.g. when specifying dependencies you only need parens for packages with constraints

 (depends
  (ocaml (>= "4.13"))
  ppx_inline_test))

Though of course there are also counter examples to this such as (no_dynlink) which requires parens.

[gridbugs]

Ah I didn't realize that the logic for expanding actions in lockfiles is in a different place to expanding actions in dune files (src/dune_rules/pkg_rules.ml vs src/dune_rules/action_unexpanded.ml). Will update.

[rgrinberg]

Staying true to lisp (scheme rather) isn't my only motivation. It's just that in dune bare words are already interchangeable with strings in all contexts. So (foo "nothing") and (foo nothing) must be equivalent. An alternative would be to have (foo :nothing) and rely on the fact that any bareword that starts with : is a symbol and can be interpreted arbitrarily by the form.

[gridbugs]

I've removed the nothing action and made it so when can only be used in lockfiles.

[gridbugs]

@rgrinberg I added a test with a single conditional action whose condition is false and I get this error:

Error: Rule dependencies are configured to require sandboxing, but the rule
has no actions that could potentially require sandboxing.
-> required by _build/default/.pkg/test/target

It seems to happen whenever a rule's action is effectively (progn). What's the point of this error? Should it be disabled in lockfiles just in case all of a package's build/install commands are filtered out?

[rgrinberg]

The point of this error is to prevent you from sandboxing rules that don't require sandboxing. Although looking at this again, it really just makes things difficult for no reason. I think dune should just not sandbox anything if there's no sandboxing required in such cases.

[rgrinberg]

For now you can workaround but always including something that needs sandboxing in the action. E.g. (system true)

[gridbugs]

I think dune should just not sandbox anything if there's no sandboxing required in such cases.

Agreed. Will workaround for now.

[Leonidas-from-XIV]

The code looks quite good, the comment about system does too (however I am a bit worried about running true on Windows - as @rgrinberg says, this this restriction can be lifted in a separate PR).

[rgrinberg]

Instead of adding the hack in the code, you could just add it in the test. E.g.

(progn
  (system "true")
  (when ...))

All real builds are going to have at least 1 run action, so this limitation isn't going to matter much.

[gridbugs]

That wouldn't test anything not already tested in the previous test so I'll just remove the empty case then.

[Leonidas-from-XIV]

Once #8491 is merged (I enabled auto-merge but not sure how this works) this can use the new helper function.

Edit: Looks like it automatically merged #8491 after I manually rebased. Ohwell, now I know how it works.