Skip to content

Commit

Permalink
internals/Template: fix bug in computing (deduplicated) deps
Browse files Browse the repository at this point in the history
Template.deps used polymorphic compare, which led to failure when
comparing templates containing workflows which can be functional
values. Unbelievable it didn't occur earlier!
  • Loading branch information
pveber committed Mar 4, 2021
1 parent f2c6cda commit d0a126a
Show file tree
Hide file tree
Showing 8 changed files with 31 additions and 11 deletions.
2 changes: 2 additions & 0 deletions lib/engine/execution_env.ml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ type insert =
}
| String of string

let compare_insert = Poly.compare

type t = {
db : Db.t ;
allowed_containers : [`Docker | `Singularity] list ;
Expand Down
2 changes: 2 additions & 0 deletions lib/engine/execution_env.mli
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ type insert =
}
| String of string

val compare_insert : insert -> insert -> int

type t = {
db : Db.t ;
allowed_containers : [`Docker | `Singularity] list ;
Expand Down
2 changes: 1 addition & 1 deletion lib/engine/shell_command.ml
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ let par x = "( " ^ x ^ " )"
))' which has a specific meaning for bash *)

let command_path_deps cmd =
Command.deps cmd
Command.deps cmd ~compare:Execution_env.compare_insert
|> List.filter_map ~f:(function
| Execution_env.Path p -> Some [ p ]
| Path_list l -> Some l.elts
Expand Down
9 changes: 5 additions & 4 deletions lib/internals/command.ml
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ type 'a t =
and 'a template = 'a Template.t


let rec deps = function
let rec deps cmd ~compare =
match cmd with
| And_list xs
| Or_list xs
| Pipe_list xs ->
List.map xs ~f:deps
List.map xs ~f:(deps ~compare)
|> List.concat
|> List.dedup_and_sort ~compare:Caml.compare
| Simple_command tokens -> Template.deps tokens
|> List.dedup_and_sort ~compare
| Simple_command tokens -> Template.deps tokens ~compare

let rec map x ~f = match x with
| Simple_command toks ->
Expand Down
2 changes: 1 addition & 1 deletion lib/internals/command.mli
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ val map :
f:('a -> 'b) ->
'b t

val deps : 'a t -> 'a list
val deps : 'a t -> compare:('a -> 'a -> int) -> 'a list
6 changes: 3 additions & 3 deletions lib/internals/template.ml
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ type 'a token =

type 'a t = 'a token list

let rec deps tmpl =
let rec deps tmpl ~compare =
List.map tmpl ~f:(function
| D r -> [ r ]
| F toks -> deps toks
| F toks -> deps ~compare toks
| S _ | DEST | TMP | NP | MEM -> []
)
|> List.concat
|> List.dedup_and_sort ~compare:Caml.compare
|> List.dedup_and_sort ~compare

let rec map_token x ~f = match x with
| S s -> S s
Expand Down
2 changes: 1 addition & 1 deletion lib/internals/template.mli
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@ val map :
f:('a -> 'b) ->
'b t

val deps : 'a t -> 'a list
val deps : 'a t -> compare:('a -> 'a -> int) -> 'a list
17 changes: 16 additions & 1 deletion lib/internals/workflow.ml
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,21 @@ let id : type s. s t -> string = function

let any x = Any x

let compare_token x y =
match x, y with
| Path_token wx, Path_token wy ->
String.compare (id wx) (id wy)
| Path_token _, _ -> -1
| Path_list_token x, Path_list_token y -> (
match String.compare (id x.elts) (id y.elts) with
| 0 -> compare (x.sep, x.quote) (y.sep, y.quote)
| i -> i
)
| Path_list_token _, _ -> -1
| String_token wx, String_token wy ->
String.compare (id wx) (id wy)
| String_token _, _ -> 1

module Any = struct
module T = struct
type t = any
Expand Down Expand Up @@ -269,7 +284,7 @@ let shell
in
let id = digest ("shell", version, digestible_cmd cmd) in
let deps = add_mem_dep mem (
Command.deps cmd
Command.deps cmd ~compare:compare_token
|> List.map (function
| Path_token w -> any w
| Path_list_token { elts ; _ } -> any elts
Expand Down

0 comments on commit d0a126a

Please sign in to comment.