feature: allow specifying which shell to use for cram test by haochenx · Pull Request #8134 · ocaml/dune

haochenx · 2023-07-06T18:36:33Z

related to Specify a specific shell to execute cram test commands #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

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

Optional?

(maybe) add environment variable to allow overriding system shell (instead of env sh) as requested in Specify a specific shell to execute cram test commands #7829

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

haochenx · 2023-07-07T10:40:19Z

@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?

Alizter · 2023-07-07T10:44:16Z

test/blackbox-tests/test-cases/cram/dune

+(cram)
+
+(cram
+ (enabled_if %{bin-available:zsh})


If you do this, you will have to add zsh to the CI so we can run the tests. Probably best if you restrict to unix too.

I think the macOS CI environment should have zsh available.

What's the additional benefits to add restriction to unix in addition to checking whether zsh exists?

Well actually I'm thinking that we shouldn't be using bin-available. If we miss putting zsh in the CI then this test will never be run, and it might surprise some other devs when they have zsh if this test is broken. So we have three choices:

Either we make zsh mandatory (hence the unix restriction) so that the test is always seen to run. We can therefore catch any changes in CI.

We make a dummy zsh or any other shell, just to check how it works. (echo would probably work)

We don't test zsh.

I see. I did have similar feeling when I added the test, but I ended up with adding them since I wanted to have some non-artificial tests.

But given that your concern is solid, I am in favor of doing (1) or (3) as you suggested. I don't think (2) is a good option since it seems that dune's cram test executor expects the shell at least have (a) output redirecting and (b) script sourcing features and making a convincing dummy zsh above the level of what is already done in shell-opt.t requires some significant efforts.

Regarding (1), I'm in general in favor of it, but since it involves changing the CI configuration, I don't think I should make the call.

If the maintainers of dune is not in favor of (1), then I think we should take (3) as missing this test case isn't crucial for the completeness of testing the feature added in this PR.

(thank you for taking a look btw!)

How should we reach to a consensus here?

johnridesabike · 2023-07-07T13:36:59Z

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 · 2023-07-07T14:28:02Z

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.

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

johnridesabike · 2023-07-07T15:04:19Z

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 · 2023-07-07T15:23:59Z

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 · 2023-07-08T13:31:22Z

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 · 2023-07-08T13:45:37Z

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 · 2023-07-08T13:58:01Z

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?

dune/src/dune_engine/action_exec.ml

Lines 289 to 301 in 4256431

    
           | System cmd -> 
        
             let path, arg = 
        
               Utils.system_shell_exn ~needed_to:"interpret (system ...) actions" 
        
             in 
        
             let+ () = exec_run ~display ~ectx ~eenv path [ arg; cmd ] in 
        
             Done 
        
           | Bash cmd -> 
        
             let+ () = 
        
               exec_run ~display ~ectx ~eenv 
        
                 (bash_exn ~loc:ectx.rule_loc ~needed_to:"interpret (bash ...) actions") 
        
                 [ "-e"; "-u"; "-o"; "pipefail"; "-c"; cmd ] 
        
             in 
        
             Done

rgrinberg · 2023-09-09T12:00:11Z

test/blackbox-tests/test-cases/cram/cram-action-shell-opt.t

+  sh.ml
+  $ cat foo.cram.corrected
+    $ echo "foo from foo.cram"
+    ***** UNREACHABLE *****


Why is this and the rest unreachable?

My guess is that when creating foo.cram, the $ is interpreted by the parent shell. You can use << 'EOF' line 69 to prevent this.

These tests try to see whether the PATH environment variable is regarded by making our own bash and sh compiled from bash.ml and sh.ml. But since those are nowhere near complete shell implementation, they do not handle stdout redirection directives (like echo hello > file.txt), which cram_exec relies on to capture the output. Therefore the cram test executor will think the cram test crashed and include ***** UNREACHABLE ***** here.

We know our own mocked version of bash and sh are invoked here by looking at https://github.com/ocaml/dune/pull/8134/files/e2913128ca6046e24b2dbfa7255be40588ce58d1#diff-0484438795287a34ad56a4a530a816caea0a5480e23fffb006408f39591b35e6R74 and https://github.com/ocaml/dune/pull/8134/files/e2913128ca6046e24b2dbfa7255be40588ce58d1#diff-0484438795287a34ad56a4a530a816caea0a5480e23fffb006408f39591b35e6R95

This is less than ideal but I didn't come up with other ways to perform this test.

rgrinberg · 2023-09-09T12:13:14Z

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 · 2023-09-13T13:01:36Z

@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 · 2023-09-20T08:09:03Z

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.

CHANGES.md

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

haochenx · 2023-09-25T03:48:39Z

otherlibs/dune-rpc/private/types.ml

  type t = int * int

-  let latest = 3, 11
+  let latest = 3, 12


not sure whether this should be in this PR

a separate PR would be best.

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

haochenx · 2023-09-25T04:04:10Z

src/dune_rules/cram/cram_rules.ml

+            | Custom_shell { prog; args } ->
+              let shell_deps, sandbox = Dep_conf_eval.unnamed ~expander [ File prog ] in
+              let dir = Path.build dir in
+              let expand_arg sw =
+                let open Action_builder.O in
+                let+ v =
+                  String_expander.Action_builder.expand
+                    sw
+                    ~dir
+                    ~mode:Many
+                    ~f:(Expander.expand_pform expander)
+                in
+                Value.L.to_strings v ~dir
+              in
+              let shell =
+                let open Action_builder.O in
+                let+ shell_args = Action_builder.all (List.map args ~f:expand_arg)
+                and+ shell_prog =
+                  Expander.(
+                    With_deps_if_necessary.expand_single_path expander prog
+                    |> Deps.action_builder)
+                in
+                shell_prog, List.concat shell_args
+              in
+              return ~shell_deps ~sandbox (Some shell)
+          in


not sure whether this is the best way to expand the prog path and arg here

haochenx · 2023-09-25T04:04:56Z

I have addressed #8134 (comment) except for %{script}, which I find a bit difficult to do.

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

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

rgrinberg · 2023-09-25T10:14:27Z

do you think %{script} is necessary?

I can give it a try myself

haochenx · 2023-09-29T12:53:57Z

@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

rgrinberg · 2023-09-30T01:19:07Z

Thanks @haochenx.

You need to use the Expander.add_bindings function to add a script binding. See how we handling %{input-file} it should be pretty similar. Maybe we should just call it %{input-file} as well. I wonder what you think about that.
That's correct.

haochenx · 2023-09-30T13:48:01Z

@rgrinberg thanks for the pointers. last time I tried to grep for input-file to find code examples, but the logic involved was quite complicated and not directly referable. I will check again.

Using Expander.add_bindings to add the binding is one thing, but suppressing the default behavior is another. When attempting, I will see which of the following approach fit the code base the best, but please review what I may do:

Maybe we should just call it %{input-file} as well. I wonder what you think about that.

My take is that it will be confusing for the users, as it's not strictly the "input file", as the script executed is quite heavily processed snippets of the original .t test description files. So sticking to %{script} is a better idea IMO.

(I estimate that I will attempt to implement %{script} in a couple of days, but no guarantee.)

Signed-off-by: Christine Rose <christinerose@users.noreply.github.com>

Allow explicitly using `bash` as the shell instead of `sh`. Extracted and/or loosely inspired form #8134 --------- Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg · 2026-01-20T22:26:24Z

We ended up providing bash as an option. Not as general as this PR, but enough to cover our needs. Feel free to extend what we have to add more shells.

haochenx force-pushed the cram-shell-spec branch from f0e7902 to f4b35a2 Compare July 6, 2023 18:36

cram_exec - add shell_spec to action spec

80a5248

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

haochenx force-pushed the cram-shell-spec branch from f4b35a2 to 80a5248 Compare July 6, 2023 18:37

haochenx marked this pull request as draft July 6, 2023 18:37

haochenx added 5 commits July 7, 2023 06:17

cram - add (shell ..) option to the (cram) stanza

506ff24

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

cram - shell-opt.t - add one test case

7273d46

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

fix tests: test/blackbox-tests/test-cases/cram/shell-opt.t

8d34734

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

add test/blackbox-tests/test-cases/cram/shell-opt-zsh.t

5850f0c

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

make fmt

96e8204

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

haochenx changed the title ~~WIP: allow specifying which shell to use for cram test~~ WIP: feature: allow specifying which shell to use for cram test Jul 6, 2023

haochenx added 3 commits July 7, 2023 16:46

move Cram_stanza.shell_spec to Dune_lang.Shell_spec.t

60de1af

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

cram - add (shell ..) option to the (cram) user action

c5724de

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

restrict (shell ..) option on (cram ..) to (lang dune 3.10) or later

6aa7773

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

Alizter reviewed Jul 7, 2023

View reviewed changes

haochenx added 2 commits July 7, 2023 23:29

cram - small fix of document

dae85d8

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

cram - add document describing (shell ..) option

6b8218a

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

haochenx force-pushed the cram-shell-spec branch from 5cc7317 to 6b8218a Compare July 7, 2023 14:29

haochenx added 2 commits July 7, 2023 23:30

Merge branch 'main' into cram-shell-spec

d05b7c0

add change log entry for ocaml#8134

3350047

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

haochenx marked this pull request as ready for review July 7, 2023 14:35

haochenx requested a review from christinerose as a code owner July 7, 2023 14:35

haochenx changed the title ~~WIP: feature: allow specifying which shell to use for cram test~~ feature: allow specifying which shell to use for cram test Jul 7, 2023

rgrinberg reviewed Sep 9, 2023

View reviewed changes

christinerose approved these changes Sep 11, 2023

View reviewed changes

Merge remote-tracking branch 'up/main' into cram-shell-spec

c686868

haochenx commented Sep 25, 2023

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

reflect review comments

fbc7b4f

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

haochenx force-pushed the cram-shell-spec branch from dd8079a to 9dbb040 Compare September 25, 2023 03:00

fix CHANGES.md

9dbb040

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

haochenx marked this pull request as draft September 25, 2023 03:01

haochenx added 3 commits September 25, 2023 05:29

bump to 3.12

25c1cc0

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

correct dune language version

c3a06f5

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

fix tests and doc

1993b95

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

haochenx commented Sep 25, 2023

View reviewed changes

no optional encoded action

544d9f2

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

haochenx commented Sep 25, 2023

View reviewed changes

fix hint text in Cram_stanza.system_shell_prog

5a8f2de

Signed-off-by: Haochen M. Kotoi-Xie <hx@kxc.inc>

haochenx marked this pull request as ready for review September 25, 2023 04:05

remove extra space

35fb63d

Signed-off-by: Christine Rose <christinerose@users.noreply.github.com>

Alizter mentioned this pull request Nov 24, 2023

Installing public C headers #9163

Open

rgrinberg mentioned this pull request Dec 28, 2025

cram: allow customizing the shell #13083

Merged

rgrinberg added a commit that referenced this pull request Dec 28, 2025

cram: allow customizing the shell (#13083)

8052503

Allow explicitly using `bash` as the shell instead of `sh`. Extracted and/or loosely inspired form #8134 --------- Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg closed this Jan 20, 2026

	module Shell_spec : sig
	type 'path t =
	\| System_shell
	\| Bash_shell
	\| Exec_file_shell of 'path

	type ('path, _) spec =
	{ script : 'path
	; shell_spec : 'path Shell_spec.t
	}

	val action : (Path.t, Import.Path.Build.t) spec -> Action.t

Conversation

haochenx commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design

Discussions

Sub-tasks

Optional?

Uh oh!

haochenx commented Jul 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haochenx Jul 7, 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!

Alizter Jul 7, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnridesabike commented Jul 7, 2023

Uh oh!

haochenx commented Jul 7, 2023

Uh oh!

johnridesabike commented Jul 7, 2023

Uh oh!

haochenx commented Jul 7, 2023

Uh oh!

haochenx commented Jul 8, 2023

Uh oh!

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

Uh oh!

Uh oh!

haochenx commented Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgrinberg commented Sep 9, 2023

Uh oh!

haochenx commented Sep 13, 2023

Uh oh!

haochenx commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haochenx commented Sep 25, 2023

Uh oh!

rgrinberg commented Sep 25, 2023

Uh oh!

haochenx commented Sep 29, 2023

Uh oh!

rgrinberg commented Sep 30, 2023

Uh oh!

haochenx commented Sep 30, 2023

Uh oh!

rgrinberg commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

haochenx commented Jul 6, 2023 •

edited

Loading

haochenx Jul 7, 2023 •

edited

Loading

Alizter Jul 7, 2023 •

edited

Loading

Alizter commented Jul 8, 2023 •

edited

Loading

haochenx commented Jul 8, 2023 •

edited

Loading

haochenx commented Sep 20, 2023 •

edited

Loading