Invalidate the cache because of a transitive dep.

On addition to direct dependencies some transitive deps could also
change between 2 commits of opam repository.

service/git_clone.ml

@@ -157,13 +157,10 @@ module Make (Cache : CacheType) = struct
     | None -> []
     | Some diff ->
       try
-        run_grep_lines ~stdin:(Eio.Flow.string_source diff) t ["^... ./packages/.*/opam"] |> Option.value ~default:[]
-        |> List.map (fun path ->
-          Astring.String.cuts ~rev:true ~sep:"/" path 
-          |> function
-          | _::_::package::_ ->  package
-          | _ -> Fmt.failwith "Pkgs diff between %s and %s of %s@." repo_url new_commit old_commit)
-      with _ -> [] (* grep could exits with status 1 *)
+        run_grep_lines ~stdin:(Eio.Flow.string_source diff) t ["^... ./packages/.*/opam"]
+        |> Option.value ~default:[]
+        |> List.filter_map (fun path -> Astring.String.cut ~sep:"/" path |> Option.map snd)
+      with _ -> [] (* grep could exits with status 1 if there's no match *)
 
   let oldest_commit_with t ~repo_url ~from paths =
     let clone_path = repo_url_to_clone_path t repo_url |> Fpath.to_string in

service/solve_cache.ml

@@ -3,7 +3,7 @@ open Eio.Std
 module Worker = Solver_service_api.Worker
 module Log_data = Solver_service_api.Solver.Log
 module Cache = Scache.Cache
-module Set = Set.Make(Worker.Selection)
+module Selections = Set.Make(Worker.Selection)
 
 exception Invalidated
 
@@ -79,7 +79,7 @@ let is_same_solution ~solve_response_cache ~solve_response =
   | Error _, _ -> false
   | _, Error _ -> false
   | Ok selections_cache, Ok selections ->
-    Set.equal (Set.of_list selections_cache) (Set.of_list selections)
+    Selections.equal (Selections.of_list selections_cache) (Selections.of_list selections)
 
 let yojson_of_list l = l |> [%to_yojson: string list]
 let yojosn_to_list l = l |> [%of_yojson: string list]
@@ -127,9 +127,9 @@ let changed_packages t ~new_opam_repo ~old_opam_repo =
              so it invalidated *)
           raise Invalidated
         | Some _, Some _ -> 
-          let pkgs = Git_clone.diff_pkgs t ~repo_url ~new_commit ~old_commit in
-          Cache.set cache ~key ~value:(Yojson.Safe.to_string (yojson_of_list pkgs));
-          pkgs
+          let pkgs_filename = Git_clone.diff_pkgs t ~repo_url ~new_commit ~old_commit in
+          Cache.set cache ~key ~value:(Yojson.Safe.to_string (yojson_of_list pkgs_filename));
+          pkgs_filename
         | None, _ ->
           Fmt.epr "The repo %s has not the commit %s@." repo_url new_commit; raise Invalidated
         | _, None ->
@@ -140,7 +140,7 @@ let changed_packages t ~new_opam_repo ~old_opam_repo =
 
 let get_names = OpamFormula.fold_left (fun a (name, _) -> name :: a) []
 
-let deps opam_pkgs =
+let deps_of_opam_file opam_pkgs =
   opam_pkgs
   |> List.map (fun (_, content) ->
     OpamFile.OPAM.read_from_string content |> OpamFile.OPAM.depends |> get_names)
@@ -152,10 +152,23 @@ let is_invalidated t ~request ~solve_cache =
     pinned_pkgs; _ } = request 
   in
   let request_pkgs =
-    List.concat_map (fun pkgs -> deps pkgs) [root_pkgs; pinned_pkgs]
+    List.concat_map (fun pkgs_name -> deps_of_opam_file pkgs_name) [root_pkgs; pinned_pkgs]
     |> List.flatten
     |> OpamPackage.Name.Set.of_list
   in
+  let response_pkgs =
+    solve_cache.Solve_cache.solve_response
+    |> Result.get_ok
+    |> List.map (fun selection -> selection.Worker.Selection.packages)
+    |> List.concat
+    |> List.map (fun pkg_version ->
+      pkg_version
+      |> Astring.String.cut ~sep:"."
+      |> Option.get
+      |> fun (name,version) ->
+        OpamPackage.create  (OpamPackage.Name.of_string name) (OpamPackage.Version.of_string version))
+    |> OpamPackage.Set.of_list
+  in
   let old_opam_repo =
     solve_cache.Solve_cache.last_opam_repository_commits
     |> List.sort (fun (url1,_) (url2,_) -> String.compare url1 url2)
@@ -169,7 +182,11 @@ let is_invalidated t ~request ~solve_cache =
   | Some pkgs ->
     pkgs
     |> List.find_opt (fun pkg ->
-      OpamPackage.Name.Set.mem (OpamPackage.Name.of_string pkg) request_pkgs)
+      OpamFilename.raw pkg
+      |> OpamPackage.of_filename
+      |> Option.get
+      |> fun opam_pkg ->
+        OpamPackage.Name.Set.mem (OpamPackage.name opam_pkg) request_pkgs || OpamPackage.Set.mem opam_pkg response_pkgs)
     |> Option.is_some
 
 (**
@@ -182,7 +199,8 @@ let is_invalidated t ~request ~solve_cache =
 
     The invalidation is about looking if the request packages is involve in
     the 2 different commit, the commit of the cached response and the commit of
-    the request.
+    the request. Also the cache response contain the transitive dependencies, we
+    make sure those ones are not also involve in the commit changes.
 
     The oldest commit used during the solve is kept when the response is the same
     as previous solve.

test/test.expected

@@ -251,78 +251,109 @@ results:
 
 ## Select foo.1.0 ##
 
-commits: [(opam-repo.git, [ocaml-base-compiler.5.0; foo.1.0])]
+commits: [(opam-repo.git,
+           [ocaml-base-compiler.5.0; bar.1.0; baz.1.0; foo.1.0; foobar.0.1])]
 root_pkgs: [app.dev]
 platforms: [debian-12-ocaml-5]
+Need to update opam-repo.git to get new commit 4733cec979c5946f667f86e0df0ac20741277d68
 results:
   [debian-12-ocaml-5:
      compat_pkgs: [app.dev]
-     packages: [app.dev; foo.1.0; ocaml-base-compiler.5.0]
-     commits: [(opam-repo.git, bf39e9df31e82a307e3daee9409f62d1a15acfe7)]
+     packages: [app.dev; bar.1.0; baz.1.0; foo.1.0; ocaml-base-compiler.5.0]
+     commits: [(opam-repo.git, 4733cec979c5946f667f86e0df0ac20741277d68)]
      lower_bound: false]
 
-## Foo 1.1 now available ##
+## Foo 1.1 now available (A direct dependency, the result will contain the new commit) ##
 
-commits: [(opam-repo.git, [foo.1.1; ocaml-base-compiler.5.0; foo.1.0])]
+commits: [(opam-repo.git,
+           [foo.1.1; ocaml-base-compiler.5.0; bar.1.0; baz.1.0; foo.1.0;
+            foobar.0.1])]
 root_pkgs: [app.dev]
 platforms: [debian-12-ocaml-5]
 Already up to date.
-Need to update opam-repo.git to get new commit 6b20fc580721bea55bdb0bf54780aaa98d162e0e
+Need to update opam-repo.git to get new commit e82813b4ed60505d8511d132e77ca062776b0b10
 results:
   [debian-12-ocaml-5:
      compat_pkgs: [app.dev]
-     packages: [app.dev; foo.1.1; ocaml-base-compiler.5.0]
-     commits: [(opam-repo.git, 6b20fc580721bea55bdb0bf54780aaa98d162e0e)]
+     packages: [app.dev; bar.1.0; baz.1.0; foo.1.1; ocaml-base-compiler.5.0]
+     commits: [(opam-repo.git, e82813b4ed60505d8511d132e77ca062776b0b10)]
      lower_bound: false]
 
-## Foo 1.1 (hit the cache, the commit won't change) ##
+## Foo 1.1 again (hit the cache, the commit won't change) ##
 
 commits: [(opam-repo.git,
-           [foo.1.1; foo.1.1; ocaml-base-compiler.5.0; foo.1.0])]
+           [foo.1.1; foo.1.1; ocaml-base-compiler.5.0; bar.1.0; baz.1.0;
+            foo.1.0; foobar.0.1])]
 root_pkgs: [app.dev]
 platforms: [debian-12-ocaml-5]
-Updating 6b20fc5..8b14733
+Updating e82813b..07bc05a
 Fast-forward
 results:
   [debian-12-ocaml-5:
      compat_pkgs: [app.dev]
-     packages: [app.dev; foo.1.1; ocaml-base-compiler.5.0]
-     commits: [(opam-repo.git, 6b20fc580721bea55bdb0bf54780aaa98d162e0e)]
+     packages: [app.dev; bar.1.0; baz.1.0; foo.1.1; ocaml-base-compiler.5.0]
+     commits: [(opam-repo.git, e82813b4ed60505d8511d132e77ca062776b0b10)]
+     lower_bound: false]
+
+## Baz 1.0 is a transitive dep of Foo, the cache will be invalidated(the result will contain the new commit) ##
+
+commits: [(opam-repo.git,
+           [baz.1.0; foo.1.1; foo.1.1; ocaml-base-compiler.5.0; bar.1.0;
+            foo.1.0; foobar.0.1])]
+root_pkgs: [app.dev]
+platforms: [debian-12-ocaml-5]
+Updating 07bc05a..86fca69
+Fast-forward
+ packages/baz/baz.1.0/opam | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+Need to update opam-repo.git to get new commit 86fca698addda8dd554cffc2ea09bf616c351332
+results:
+  [debian-12-ocaml-5:
+     compat_pkgs: [app.dev]
+     packages: [app.dev; bar.1.0; baz.1.0; foo.1.1; foobar.0.1;
+                ocaml-base-compiler.5.0]
+     commits: [(opam-repo.git, 86fca698addda8dd554cffc2ea09bf616c351332)]
      lower_bound: false]
 
-## Oof 1.0 now available (hit the cache, the commit won't change) ##
+## Oof 1.0 now available (hit the cache, the commit won't change in the result) ##
 
 commits: [(opam-repo.git,
-           [oof.1.0; oof.1.0; foo.1.1; ocaml-base-compiler.5.0; foo.1.0])]
+           [oof.1.0; baz.1.0; foo.1.1; foo.1.1; ocaml-base-compiler.5.0;
+            bar.1.0; foo.1.0; foobar.0.1])]
 root_pkgs: [app.dev]
 platforms: [debian-12-ocaml-5]
-Updating 8b14733..fbfff59
+Updating 86fca69..0eb9328
 Fast-forward
  packages/oof/oof.1.0/opam | 4 ++++
  1 file changed, 4 insertions(+)
  create mode 100644 packages/oof/oof.1.0/opam
 results:
   [debian-12-ocaml-5:
      compat_pkgs: [app.dev]
-     packages: [app.dev; foo.1.1; ocaml-base-compiler.5.0]
-     commits: [(opam-repo.git, 6b20fc580721bea55bdb0bf54780aaa98d162e0e)]
+     packages: [app.dev; bar.1.0; baz.1.0; foo.1.1; foobar.0.1;
+                ocaml-base-compiler.5.0]
+     commits: [(opam-repo.git, 86fca698addda8dd554cffc2ea09bf616c351332)]
      lower_bound: false]
 
-## Oof 1.1 now available (invalidate the cache, foo 1.1 will be removed, the new commit will be taken for the result) ##
+## Oof 1.1 now available (will invalidate the cache because foo 1.1 will be removed, the result will contain the new commit) ##
 
-commits: [(opam-repo.git, [oof.1.1; ocaml-base-compiler.5.0; foo.1.0])]
+commits: [(opam-repo.git,
+           [oof.1.1; ocaml-base-compiler.5.0; bar.1.0; baz.1.0; foo.1.0;
+            foobar.0.1])]
 root_pkgs: [app.dev]
 platforms: [debian-12-ocaml-5]
-Updating fbfff59..df311eb
+Updating 0eb9328..44d1c6f
 Fast-forward
- packages/oof/oof.1.0/opam                  | 4 ----
- packages/{foo/foo.1.1 => oof/oof.1.1}/opam | 0
- 2 files changed, 4 deletions(-)
- delete mode 100644 packages/oof/oof.1.0/opam
- rename packages/{foo/foo.1.1 => oof/oof.1.1}/opam (100%)
+ packages/baz/baz.1.0/opam              | 2 +-
+ packages/foo/foo.1.1/opam              | 4 ----
+ packages/oof/{oof.1.0 => oof.1.1}/opam | 0
+ 3 files changed, 1 insertion(+), 5 deletions(-)
+ delete mode 100644 packages/foo/foo.1.1/opam
+ rename packages/oof/{oof.1.0 => oof.1.1}/opam (100%)
+Need to update opam-repo.git to get new commit 44d1c6f9bf942a6ea6719e70e0f1340cfb6b3c9e
 results:
   [debian-12-ocaml-5:
      compat_pkgs: [app.dev]
-     packages: [app.dev; foo.1.1; ocaml-base-compiler.5.0]
-     commits: [(opam-repo.git, 6b20fc580721bea55bdb0bf54780aaa98d162e0e)]
+     packages: [app.dev; bar.1.0; baz.1.0; foo.1.0; ocaml-base-compiler.5.0]
+     commits: [(opam-repo.git, 44d1c6f9bf942a6ea6719e70e0f1340cfb6b3c9e)]
      lower_bound: false]

test/test.ml

@@ -235,35 +235,50 @@ let test_solve_cache t =
   let solve = solve_cache in
   let opam_repo = Opam_repo.create "opam-repo.git" in
   let root_pkgs = ["app.dev", {| depends: [ "foo" ] |}] in
+  let depends = {| depends: [ "ocaml-base-compiler" "bar" ] |} in
   let opam_packages = [
     "ocaml-base-compiler.5.0", "";
-    "foo.1.0", {| depends: [ "ocaml-base-compiler" ] |};
+    "bar.1.0", {| depends: [ "baz" ] |};
+    "baz.1.0", "";
+    "foo.1.0",depends;
+    "foobar.0.1", "";
   ]
   in
   let first_opam_packages = opam_packages in
   let recent_commits =
     solve t "Select foo.1.0" ~platforms ~root_pkgs ~previous_commits:[opam_repo,[]]
       ~commits:[opam_repo, opam_packages]
   in
-  let opam_packages = ("foo.1.1","") :: opam_packages in
+  let opam_packages = ("foo.1.1",depends) :: opam_packages in
   let recent_commits =
-  solve t "Foo 1.1 now available" ~previous_commits:recent_commits ~platforms ~root_pkgs ~commits:[
+  solve t "Foo 1.1 now available (A direct dependency, the result will contain the new commit)" ~previous_commits:recent_commits ~platforms ~root_pkgs ~commits:[
     opam_repo, opam_packages
   ]
   in
+  let opam_packages = ("foo.1.1",depends)::opam_packages in
   let recent_commits =
-    solve t "Foo 1.1 (hit the cache, the commit won't change)" ~previous_commits:recent_commits ~platforms ~root_pkgs ~commits:[
-      opam_repo, ("foo.1.1","")::opam_packages
+    solve t "Foo 1.1 again (hit the cache, the commit won't change)" ~previous_commits:recent_commits ~platforms ~root_pkgs ~commits:[
+      opam_repo, opam_packages
     ]
   in
+  let opam_packages =
+   ("baz.1.0",{|depends: [ "foobar" ]|})
+    ::(List.remove_assoc "baz.1.0" opam_packages)
+  in
+  let recent_commits =
+    solve t "Baz 1.0 is a transitive dep of Foo, the cache will be invalidated(the result will contain the new commit)" ~previous_commits:recent_commits ~platforms ~root_pkgs ~commits:[
+      opam_repo, opam_packages
+    ]
+  in
+
   let opam_packages = ("oof.1.0","") :: opam_packages in
   let recent_commits =
-  solve t "Oof 1.0 now available (hit the cache, the commit won't change)" ~previous_commits:recent_commits ~platforms ~root_pkgs ~commits:[
-    opam_repo, ("oof.1.0","") :: opam_packages
+  solve t "Oof 1.0 now available (hit the cache, the commit won't change in the result)" ~previous_commits:recent_commits ~platforms ~root_pkgs ~commits:[
+    opam_repo, opam_packages
   ]
   in
   solve t
-    "Oof 1.1 now available (invalidate the cache, foo 1.1 will be removed, the new commit will be taken for the result)"
+    "Oof 1.1 now available (will invalidate the cache because foo 1.1 will be removed, the result will contain the new commit)"
     ~previous_commits:recent_commits ~platforms ~root_pkgs ~commits:[
     opam_repo, ("oof.1.1","") :: first_opam_packages
   ] |> ignore;