Add `when` action available in lockfiles by gridbugs · Pull Request #8443 · ocaml/dune

gridbugs · 2023-08-21T07:28:03Z

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 · 2023-08-21T17:45:30Z

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

gridbugs · 2023-08-22T01:29:55Z

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 · 2023-08-22T01:43:01Z

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 · 2023-08-22T01:46:43Z

+     (progn
+      (echo "You're using ")
+      (when (= %{system} linux) (echo "Linux!"))
+      (when (= %{system} macosx) (echo "MacOS!")))


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.

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?

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?

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.

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

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

gridbugs · 2023-08-22T03:34:38Z

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 · 2023-08-22T07:48:21Z

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 · 2023-08-22T07:50:10Z

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

gridbugs · 2023-08-22T15:28:36Z

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 · 2023-08-22T15:30:45Z

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 · 2023-08-22T15:51:25Z

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 · 2023-08-23T05:30:08Z

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

gridbugs · 2023-08-23T07:01:01Z

@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 · 2023-08-24T11:03:29Z

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 · 2023-08-24T11:04:06Z

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

gridbugs · 2023-08-24T11:08:30Z

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 · 2023-08-24T13:13:19Z

+  (* An action which has no effect. Note that we can't use [Action.Progn []]
+     here because currently every rule must have at least one action that
+     requires sandboxing. *)
+  let action_noop = Action.System "true"


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.

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

Leonidas-from-XIV · 2023-08-25T08:33:12Z

+  >    (run echo e))))
+  > EOF
+
+  $ dune build .pkg/test/target/


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.

This will be used to implement opam filters on commands. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

gridbugs force-pushed the actions-when-and-nothing branch 2 times, most recently from 25f6b1d to 8bbf622 Compare August 21, 2023 07:29

gridbugs requested review from Leonidas-from-XIV, emillon and rgrinberg and removed request for emillon August 21, 2023 07:29

gridbugs added the package management label Aug 21, 2023

Alizter reviewed Aug 22, 2023

View reviewed changes

gridbugs force-pushed the actions-when-and-nothing branch from 8bbf622 to 94f68b4 Compare August 22, 2023 03:35

gridbugs force-pushed the actions-when-and-nothing branch 2 times, most recently from fe1cc21 to c445133 Compare August 23, 2023 05:26

gridbugs changed the title ~~Add when and nothing actions~~ Add when action availabel in lockfiles Aug 23, 2023

gridbugs changed the title ~~Add when action availabel in lockfiles~~ Add when action available in lockfiles Aug 23, 2023

gridbugs force-pushed the actions-when-and-nothing branch 2 times, most recently from c45e3f7 to a5bb60e Compare August 23, 2023 06:57

gridbugs force-pushed the actions-when-and-nothing branch from a5bb60e to f309a25 Compare August 24, 2023 06:49

gridbugs mentioned this pull request Aug 24, 2023

pkg: add run-with-conditional-terms action #8486

Closed

gridbugs force-pushed the actions-when-and-nothing branch from f309a25 to 73da3a0 Compare August 24, 2023 11:14

Leonidas-from-XIV approved these changes Aug 24, 2023

View reviewed changes

Comment thread test/blackbox-tests/test-cases/pkg/pkg-action-when.t Outdated

rgrinberg reviewed Aug 24, 2023

View reviewed changes

gridbugs force-pushed the actions-when-and-nothing branch from 73da3a0 to 0f48ed4 Compare August 25, 2023 05:23

Leonidas-from-XIV reviewed Aug 25, 2023

View reviewed changes

gridbugs mentioned this pull request Aug 25, 2023

pkg: convert opam command filters to dune blangs #8502

Merged

gridbugs and others added 2 commits August 25, 2023 12:18

Add when action available in lockfiles

8b2c101

This will be used to implement opam filters on commands. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

_

66088be

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg force-pushed the actions-when-and-nothing branch from 0f48ed4 to 66088be Compare August 25, 2023 11:21

rgrinberg merged commit 87ae04a into ocaml:main Aug 25, 2023

Alizter mentioned this pull request Oct 5, 2023

rfc(engine): sandbox only when needed rather than throwing an error #8854

Closed

gridbugs deleted the actions-when-and-nothing branch October 11, 2023 03:44

Conversation

gridbugs commented Aug 21, 2023

Uh oh!

Alizter commented Aug 21, 2023

Uh oh!

gridbugs commented Aug 22, 2023

Uh oh!

Alizter commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alizter Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

gridbugs Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gridbugs Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

rgrinberg Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

gridbugs Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

rgrinberg Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

gridbugs commented Aug 22, 2023

Uh oh!

rgrinberg commented Aug 22, 2023

Uh oh!

rgrinberg commented Aug 22, 2023

Uh oh!

gridbugs commented Aug 22, 2023

Uh oh!

gridbugs commented Aug 22, 2023

Uh oh!

rgrinberg commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gridbugs commented Aug 23, 2023

Uh oh!

gridbugs commented Aug 23, 2023

Uh oh!

rgrinberg commented Aug 24, 2023

Uh oh!

rgrinberg commented Aug 24, 2023

Uh oh!

gridbugs commented Aug 24, 2023

Uh oh!

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rgrinberg Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

gridbugs Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

Leonidas-from-XIV Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Alizter commented Aug 22, 2023 •

edited

Loading

gridbugs Aug 22, 2023 •

edited

Loading

rgrinberg commented Aug 22, 2023 •

edited

Loading

Leonidas-from-XIV Aug 25, 2023 •

edited

Loading