feat(cram): timeout for cram tests

[Alizter]

We add a (timeout) field for cram stanzas allowing users to set up a time budget for cram tests to run in. If the budget is exceeded then the cram test will be cancelled.

• dune_engine changes are ok?
• changelog
• documentation

[rgrinberg]

You should pass this loc as a tuple with the timeout so that we have an idea what the loc applies to.

[Alizter]

We have timeout : (Loc.t * float) option here whereas loc is just the cram test file:

      let loc = Loc.in_file (Path.drop_optional_build_context_maybe_sandboxed src) in

I've gone ahead and passed the src : Path.t so that we can infer the location from that directly rather than early on.

[rgrinberg]

I'd like to review how the digest is computed later this week

[Alizter]

@rgrinberg I suppose since we are doing this:

    let encode { src; dir; script; output; timeout } path target : Sexp.t =
      List
        [ path src
        ; path dir
        ; path script
        ; target output
        ; Dune_sexp.Encoder.(option float (Option.map ~f:snd timeout))
          |> Dune_sexp.to_sexp
        ]
    ;;

we are changing the rule digest every time we change the time. This is confirmed by the test I just pushed: cram/timeout-redigest.t.

Reading build_system.ml I see that we compute rule digests as:

    let trace =
      ( rule_digest_version (* Update when changing the rule digest scheme. *)
      , sandbox_mode
      , Dep.Facts.digest facts ~env
      , target_paths rule.targets.files @ target_paths rule.targets.dirs
      , Action.for_shell action
      , can_go_in_shared_cache
      , List.map locks ~f:Path.to_string
      , Execution_parameters.action_stdout_on_success execution_parameters
      , Execution_parameters.action_stderr_on_success execution_parameters
      , Execution_parameters.workspace_root_to_build_path_prefix_map execution_parameters
      , Execution_parameters.expand_aliases_in_sandbox execution_parameters )
    in
    Digest.generic trace

and Action.for_shell uses encode. Therefore the encoding directly affects what we digest.

If we deem that timeout changes shouldn't affect digests then the simple solution would be to remove the timeout from the digest, but I am unsure if this is something we want.

Take the situation where we have a test that succeeds with no timeout, then we add a short timeout that shouldn't succeed. It will not rerun the rule since the digest is the same, but obviously that build can no longer be reproduced.

A more sensible semantic would be for the digest not to change for larger timeouts, but to change for smaller timeouts. However such a thing seems impossible.

[rgrinberg]

Re-running the action when the timeout changes is fine, the issue is with allowing the action to observe the name of cram script. That seems very much unnecessary. I've gone ahead and gotten rid of it.

[Alizter]

Since we just branched 3.20, we need to wait for the 3.21 version bump.

[rgrinberg]

Why does it matter? 3.20 hasn't been released yet.

[Alizter]

Right, but the alpha has started, so if we merge this now, we need to remember to bump the version numbers to 3.21 since this patch won't be in 3.20. AFAIK we don't backport new PRs after the alpha.

[rgrinberg]

Can't we always just do another alpha? In any case, I'll leave the choice to @maiste to either include this in 3.20 or 3.21.

[maiste]

If that's mandatory I can redo an alpha1. What do you prefer?

[rgrinberg]

I think users would certainly appreciate being able to use this feature sooner rather than later.

So if it's possible, yes I would do another alpha with this feature.

[maiste]

OK then let's go with this. It will allow me to fix some typos that were in the previous changelog.