Fix detection of dynamic cycles #988
Conversation
ghost
requested review from
| ; mode = Standard | ||
| ; loc = None | ||
| ; dir = Path.root | ||
| ; exec = Not_started { eval_rule = (fun _ -> assert false) |
There was a problem hiding this comment.
Is there any way to encode this into a new constructor so that these assertions can be removed?
There was a problem hiding this comment.
I'm not sure how to interpret it though. This rule represent the build goal, so it should only terminate when the build is done. We could also create a new rule for every build and make it depend on the requested target. I'm having a look
There was a problem hiding this comment.
This is a bit annoying to change actually, so I just added a comment for now. We can look at this in another PR
There was a problem hiding this comment.
OK! that's good like this.
src/build_system.ml
Outdated
| List.find t.rev_deps ~f:(fun (_, t) -> | ||
| Path.Set.mem t.all_targets last) | ||
| with | ||
| | None -> assert false |
There was a problem hiding this comment.
We can use Option.value_exn here (there's a couple List.find uses like this in the code base, so extracting a List.find_exn is a nice alternative as well)
There was a problem hiding this comment.
Indeed. I'm using Option.value_exn for now, we can add List.find_exn and change all the occurrences at once in another PR
There was a problem hiding this comment.
OK I'll take care of this part.
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
It's a smaller set and comparison is quicker Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
ghost
mentioned this pull request
This PR fixes an issue we observed yesterday with @xclerc where dune silently exit with an error code. The problem was in fact a problem with the detection of cycles that appear dynamically due to dynamic dependencies.
This PR adds as regression test the case we found + two other cases. It then fixes the issue by keeping track of all reverse dependency paths.