feature: allow specifying which shell to use for cram test

[haochenx]

• related to https://github.com/ocaml/dune/issues/7829
• rendered doc: https://dune--8134.org.readthedocs.build/en/8134/tests.html#test-options

Design

Add (shell ..) option to the (cram) stanza and user action. The syntax is described below:

• (shell :system) will use the system shell (aka /usr/bin/env sh)
• (shell :bash) will use bash (aka /usr/bin/env bash)
• (shell <dep>) will resolve <dep> (that may contain variables) as a path to an executable and use it as the shell
  • e.g. (shell %{bin:zsh}) will use zsh as the shell
  • e.g. (shell %{bin:dash}) will use dash as the shell

Discussions

• [x] Ast of cram action is changed (to accommodate specifying shell_spec)
  • this might be a breaking change for external tools relying on output of dune rules
  • it seems that dune rules does not output entries regarding cram tests so we can probably safely assume that currently no external tools depends on the encoded action spec

Sub-tasks

• [x] allow config of shell for Cram_exec.action (done in https://github.com/ocaml/dune/pull/8134/commits/80a5248e69736b013758fba3e6202d9679d5b98a)
  • https://github.com/ocaml/dune/blob/80a5248e69736b013758fba3e6202d9679d5b98a/src/dune_rules/cram/cram_exec.mli#L3-L7
  • https://github.com/ocaml/dune/blob/80a5248e69736b013758fba3e6202d9679d5b98a/src/dune_rules/cram/cram_exec.mli#L14-L19
• [x] add stanza syntax (done in https://github.com/ocaml/dune/pull/8134/commits/506ff248deef758bf0c63d640b195d31f0954dcb)
  • (cram (shell <shell_spec>)) where <shell_spec> could be
    • :system, which is interpreted as System_shell (aka env sh)
    • :bash, which is interpreted as Bash_shell (aka env bash)
    • <dep>, which is interpreted the same way as those in (deps <dep>)
• [x] add rule action syntax (done in https://github.com/ocaml/dune/pull/8134/commits/c5724dea8f2de6c46a85685ed776ad9fac6586df)
  • seems like an undocumented feature?
• [x] add new test cases
  • [x] for stanza syntax (test/blackbox-tests/test-cases/cram/{shell-opt,shell-opt-zsh}.t added)
  • [x] for rule action syntax (test/blackbox-tests/test-cases/cram/cram-action-shell-opt.t added)
  • [x] for min dune lang version (test/blackbox-tests/test-cases/cram/shell-opt-dune-lang-version.t added)
• [x] check for dune language version (>= 3.10)
• [x] modify documentation
• [x] add change log

Optional?

• [ ] (maybe) add environment variable to allow overriding system shell (instead of env sh) as requested in https://github.com/ocaml/dune/issues/7829

[haochenx]

@rgrinberg do you mind to take a look at the design and implementation of this PR

@johnridesabike do you think this approach satisfy your use case?

[johnridesabike]

This current implementation would certainly be helpful, but using an environment variable or CLI argument would be preferable for my use case.

My goal is to check that my tests are shell-agnostic, so I'd like to run them in something like dash locally (or in multiple different shells), but allow other users/CIs run them in whatever they have installed. I wouldn't want to commit a Dune rule that requires using a specific shell.

[haochenx]

It's probably not what you want, but since you can refer to environment variable, you could probably achieve what you wanted to achieve.

For example, you can use (shell %{env:DUNE_CRAM_SHELL=/bin/sh}) to achieve what you described.

Yet it is indeed not as elegant. I will probably look into using an environment variable, but I think we need to set the expected behavior straight first I don't think I will be adding it in this PR.

[johnridesabike]

For example, you can use (shell %{env:DUNE_CRAM_SHELL=/bin/sh}) to achieve what you described.

Wouldn't this require all users to have that environment variable set in order to run cram tests?

If we can only configure it in a Dune file, then I'll just add a (shell ...) stanza locally and not commit it. That's not necessarily a problem, just a little clunky. It's still better than not having the feature at all though.

[haochenx]

For example, you can use (shell %{env:DUNE_CRAM_SHELL=/bin/sh}) to achieve what you described.

Wouldn't this require all users to have that environment variable set in order to run cram tests?

The =/bin/sh part provides the default value (in this case, /bin/sh) so I guess not all users need to specify that environment variable.

I agree that being able to specify the shell elegantly via environment variable is a great feature.

I will think about how the spec.

[haochenx]

I'm thinking about to add two environment variables that matters for dune:

• DUNE_CRAM_SYSTEM_SHELL, which if set, decides the path when Shell_spec.System_shell is selected
• DUNE_CRAM_BASH_SHELL, which if set, decides the path when Shell_spec.Bash_shell is selected

As it comes for determining the actual path for Shell_spec.System_shell (and respectively for Shell_spec.Bash_shell), the following logic is to be used:

• check whether envar DUNE_CRAM_SYSTEM_SHELL exists and being non-empty, if so
  • if the given path is an absolute path, check its existence and use it as the shell program
    • if the path does not exists or is a directory, issue an error
  • if the given path is a simple name, resolve it with Stdune.Bin.which
    • if this fails, issue an error
  • otherwise, issue an error
• otherwise, use the current behavior (viz. resolve "sh" with Stdune.Bin.which)

Comments are welcomed.

[Alizter]

I would go for something more general. We have system and bash actions that users can put in rules. We should just have DUNE_BASH_SHELL and DUNE_SYSTEM_SHELL configure these also, not just cram. However that would make the scope of this PR bigger than it needs to be.

[haochenx]

I would go for something more general. We have system and bash actions that users can put in rules. We should just have DUNE_BASH_SHELL and DUNE_SYSTEM_SHELL configure these also, not just cram. However that would make the scope of this PR bigger than it needs to be.

Sounds better! I agree that this change will go beyond the scope of this PR. I'm happy to submit another PR to implement that. If my understanding is correct, the place also need to be modify should be the following? https://github.com/ocaml/dune/blob/4256431b1b8aa25bf9cac578286a9142c6e1c3cb/src/dune_engine/action_exec.ml#L289-L301

[Alizter]

Yes that would be one place, but there are also other internal places (such as when dune talks to VCS or runs the compiler to find config stuff). I am not certain that we want to touch all of these. The design of such a feature would need some thinking. Keeping to just the cram case I think is fine for now, and if we want to generalise then we can. At the moment, I don't have a good enough use case for such broad strokes.

[haochenx]

Make sense. But I still think we should probably do the same thing for (system ..) / (bash ..) and for cram. If we add DUNE_CRAM_XXX_SHELL and generalize later, we have to deal with the possibility of DUNE_CRAM_XXX_SHELL being different than the generalized config.

What about to have ~DUNE_USER_ACTION_SYSTEM/BASH_SHELL~ DUNE_ACTION_SYSTEM/BASH_SHELL and have them to govern around bond (a) cram test and (b) (system ..) and (bash ..)?

[Alizter]

In any case, I don't think adding the env var should be done in this PR. That can come at a later time.

[haochenx]

In any case, I don't think adding the env var should be done in this PR. That can come at a later time.

Agreed. I'm also not planning doing that in this PR. I will be opening another PR soon regarding env var and we should be able to continue the discussion there.

[haochenx]

PR dealing with the environment variables:

• https://github.com/ocaml/dune/pull/8149

[Alizter]

I am also inclined to suggest that you document the cram user action, but I am unsure if this was omitted on purpose. @emillon could probably say more.

[haochenx]

I am also inclined to suggest that you document the cram user action, but I am unsure if this was omitted on purpose.

When adding cram-action-shell-opt.t, I'm actually surprised by the behavior of the (cram) action, where

• the only thing (cram foo.tt) action will do is to generate a foo.t.corrected file, and
  • having a stanza like (rule (action (cram foo.tt))) will raise Error: Rule has no targets specified
    • (rule (target foo.tt.corrected) (cram foo.tt)) works fine
  • having a stanza like (rule (alias runtest) (action (cram foo.tt))) will result in a correct dune config, but will always succeed even if foo.tt and foo.tt.corrected differs.
    • it seems that the "correct" way to run tests is to
      • either use two stanzas (rule (target foo.tt.corrected) (action (cram foo.tt))) + (rule (alias runtest) (action (diff foo.tt.corrected foo.tt)))
      • or use (progn) to combine (cram) and (diff), like (rule (alias runtest) (action (progn (cram foo.tt) (diff foo.tt.corrected foo.tt))))
      • these will fail the test if there is a diff, but dune promote does not work
• there seems to be no way to promote foo.tt.corrected back to foo.tt.

The behavior seems a bit difficult to explain clearly (maybe not even to justify well) in documents to me.

[haochenx]

CI error after rebasing main seems to be the same as main

• this PR: https://github.com/ocaml/dune/actions/runs/5514622104/jobs/10054087615?pr=8134
• main: https://github.com/ocaml/dune/actions/runs/5511847828/jobs/10047934514

Run opam exec -- make test
./_boot/dune.exe runtest
File "test/blackbox-tests/test-cases/github8[15](https://github.com/ocaml/dune/actions/runs/5514622104/jobs/10054087615?pr=8134#step:11:16)8.t", line 1, characters 0-0:
/usr/local/bin/git --no-pager diff --no-index --color=always -u _build/.sandbox/883412a09d29c61c1a8403e31dfaa09b/default/test/blackbox-tests/test-cases/github8158.t _build/.sandbox/883412a09d29c61c1a8403e31dfaa09b/default/test/blackbox-tests/test-cases/github8158.t.corrected
diff --git a/_build/.sandbox/883412a09d29c61c1a8403e31dfaa09b/default/test/blackbox-tests/test-cases/github8158.t b/_build/.sandbox/883412a09d29c61c1a8403e31dfaa09b/default/test/blackbox-tests/test-cases/github8158.t.corrected
index 0e58359..54c0530 100644
--- a/_build/.sandbox/883412a09d29c61c1a8403e31dfaa09b/default/test/blackbox-tests/test-cases/github8158.t
+++ b/_build/.sandbox/883412a09d29c61c1a8403e31dfaa09b/default/test/blackbox-tests/test-cases/github8158.t.corrected
@@ -37,6 +37,6 @@ This fails, but should work (and print nothing):
 
   $ dune exec bin/e.exe 2>&1 | sed -e 's/camlppx....../camlppxXXXXXX/g' -e 's/build_......_dune/build_XXXXXX_dune/g'
   File "bin/.e.eobjs/_unknown_", line 1, characters 0-0:
-  sh: 1: ../.ppx/059965dd2a[17](https://github.com/ocaml/dune/actions/runs/5514622104/jobs/10054087615?pr=8134#step:11:18)61d0e00c111505aef5ac/ppx.exe: not found
+  sh: ../.ppx/059965dd2a1761d0e00c111505aef5ac/ppx.exe: No such file or directory
   File "bin/e.ml", line 1:
-  Error: I/O error: ../.ppx/059965dd2a1761d0e00c111505aef5ac/ppx.exe --as-ppx '/tmp/build_XXXXXX_dune/build_XXXXXX_dune/camlppxXXXXXX' '/tmp/build_XXXXXX_dune/build_XXXXXX_dune/camlppxXXXXXX'
+  Error: I/O error: ../.ppx/059965dd2a1761d0e00c111505aef5ac/ppx.exe --as-ppx '/var/folders/[24](https://github.com/ocaml/dune/actions/runs/5514622104/jobs/10054087615?pr=8134#step:11:25)/8k48jl6d249_n_qfxwsl6xvm0000gn/T/build_XXXXXX_dune/build_XXXXXX_dune/camlppxXXXXXX' '/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/build_XXXXXX_dune/build_XXXXXX_dune/camlppxXXXXXX'

[Alizter]

@haochenx Don't worry about that error. A test demonstrating an issue #8158 and a fix #8159 were merged in succession however the failure in the test was not 100% reproducible but went away after the fix.

[Alizter]

By the way, during last weeks dev meeting we discussed this feature: https://github.com/ocaml/dune/wiki/dev-meeting-20230712

I am not certain if we drew any conclusions however, perhaps @emillon can say more.

[haochenx]

@Alizter Thank you for the info! I have look at the meeting notes and here are some comments of mine:

on windows, it makes us rely on cygwin

Not completely sure, but I think the current implementation in a way expect a *nix like shell so we probably are already relying on cygwin. (and I think there are some code that detects windows and performs cygwin-specific path translation)

possible way to make smaller feature: just allow bash?

Totally reasonable. I won't mind shredding this PR down to this. Also it would (hypothetically) prevent writing portable tests for minimal environment (e.g. busybox)

Though I don't think taking the step to allow arbitrary shell will greatly decrease the complexity of this PR..

Overall I do feel this PR is unexpectedly a large diff for its scope and I am in favor of reducing the feature (i.e. restrict it to "bashism" as it's the most useful feature this PR introduce for our use case)

[Alizter]

I think one thing that wasn't in the notes was the worry about giving users a foot gun with being able to select shells. I think we will probably discuss it again at the next meeting to understand what exactly we want out of this feature, since that wasn't clear for us.

[haochenx]

Make sense. I can definitely wait.

For your reference at least the motivation problem we got is that since currently cram test uses which sh it's is undecided what shell you get.

On our dev machines (mostly macOS) we get bash but on our CI machine we get csh. And that is causing our test cases to fail.

[rgrinberg]

There's no problem with being able to select the shell. Portability isn't a universal concern and it's not the job of the build system to force it down our users. Bash isn't the be-all and end-all of shells and there's plenty of reasons to want to use something else.

I don't really like the configuration introduced in this PR though. I would imagine this feature to be configurable through the cram stanza. So that it's possible to set once for the entire test suite. Allowing to be customizable in the action is hardly useful in comparison.

Also, the special symbols for bash and system seem gratuitous. Is providing a binary name looking it up in PATH doesn't work for your use case?

[haochenx]

re: https://github.com/ocaml/dune/pull/8134#issuecomment-1646859986

First of all, I'm open for syntactical suggestions. To answer your questions and provide more contexts:

I would imagine this feature to be configurable through the cram stanza. So that it's possible to set once for the entire test suite.

It's possible and actually the main motivation for this PR.

Allowing to be customizable in the action is hardly useful in comparison.

Allowing the customization in the user action is a bonus for completeness of implementation. I wasn't even aware about this feature before starting working on this PR. I was thinking about omitting it at the beginning, but ended up implementing the same level of support just for the completeness.

I'd be glad to drop the user action support if that helps reaching an agreement and accelerate the review of this PR.

Also, the special symbols for bash and system seem gratuitous. Is providing a binary name looking it up in PATH doesn't work for your use case?

Without special symbols, I'd imagine one'd use (shell bash) for bash and (shell system) for the system shell. This raise ambiguity though since we don't know whether we should use dune's dependency language to expand bash (or system, respectively) or should lookup bash (or sh, respectively) using which.

I opted for special symbols to avoid the ambiguity in syntax.

• For :bash, one can definitely use %{bin:bash} instead so my implementation clearly embeds a favoritism towards bash. But this has been in the other places in dune, and I think wanting bash is a common enough motivating use case for this feature, so this favoritism is justifiable. Nevertheless I'm not too against getting rid of it.

• For :system, it's a bit trickier. We could convince ourselves to assume the system shell will always be equivalent to %{bin:sh}, but if (hypothetically) e.g. we add "proper" support for windows and use cmd when this option is not present, then the assumption does not hold. In this situation we do not have a way to explicitly specify the "default" behavior.

  Maybe trading :system with :default is more palatable to you. I do not really have a strong preference here.

  I chose :system merely because there is a user action called system. There is also another user action called bash, which made me thought choosing :bash and :system creates some good consistency.

[haochenx]

There's no problem with being able to select the shell. Portability isn't a universal concern and it's not the job of the build system to force it down our users. Bash isn't the be-all and end-all of shells and there's plenty of reasons to want to use something else.

I believe I'm holding the same view in general. As for giving bash special treatment, I think it's a good trade-off for our current time. It's almost everywhere, and you'd get a good deal of portability when specifying it over gambling what the system shell would be.

So I think it's useful to give bash some favoritism, but at the same time not against getting rid of its special treatments.

[rgrinberg]

The problem with the special symbols is twofold:

• The dash and oil shells are just as useful as zsh and bash.
• You're not allowing users to customize the command line arguments for running the shell

This raise ambiguity though since we don't know whether we should use dune's dependency language to expand bash (or system, respectively) or should lookup bash (or sh, respectively) using which.

Do you mean the dependency language or for allowing percent forms? I assume you meant the latter because that's the only thing that makes sense in this context. I think we should allow for expanding dune's percent forms and we should also allow command line arguments as well. For example, one should be able to:

(cram
 (shell %{bin:dash} -v %{script})) ;; %{script} will be expanded to the path of the shell script. like %{input-file} for ppx.

Dune has a single way of resolving binaries so the question regarding which are automatically resolved. E.g. see how Run is expanded in action_unexpanded.ml.

Once this is resolved and you remove the optional argument, this PR is good to go.

[haochenx]

@rgrinberg @emillon thanks for having a look and point out a direction. I will take sometime later this week to re-look at this issue and bring it forward.

[haochenx]

Unfortunately (and my apologies for that) I didn't get time to work on this PR last week.. I will see whether I can find some time this weekend.

[haochenx]

I have addressed https://github.com/ocaml/dune/pull/8134#issuecomment-1712497305 except for %{script}, which I find a bit difficult to do.

@rgrinberg do you think %{script} is necessary?

[rgrinberg]

do you think %{script} is necessary?

I can give it a try myself

[haochenx]

@rgrinberg regarding %{script}, I could probably attempt if you could kindly (1) point me to code that demonstrates how it could be implement (2) confirm the exact semantics that we want: I assume that we should stop appending the %{script} automatically when one is specified