pkg: respect avoid-versions between versions of pkg (#10642)

* pkg: respect avoid-versions between versions of pkg

Dune vendors the opam-0install-solver package and uses it to solve
package dependencies, however opam-0install-solver does not respect
the avoid-versions flag of opam packages. This can lead to situations
where unstable releases of packages (notably of the compiler packages)
are chosen in favor of the latest stable release of a package.

This change implements a partial workaround for this limitation where
the solver will prefer versions of packages that lack the
avoid-version flag.

Note that this does not fully implement the avoid-versions flag. If a
package depends on a disjunction of different packages then the
avoid-versions flag is not considered when choosing which package in
the disjunction to choose.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

* Rework compare code according to review

Signed-off-by: Marek Kubica <marek@tarides.com>

---------

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Marek Kubica <marek@tarides.com>
Co-authored-by: Marek Kubica <marek@tarides.com>

src/dune_pkg/opam_solver.ml

@@ -32,6 +32,10 @@ let add_self_to_filter_env package env variable =
     else env variable
 ;;
 
+let opam_file_is_avoid_version (opam_file : OpamFile.OPAM.t) =
+  List.mem opam_file.flags Pkgflag_AvoidVersion ~equal:Poly.equal
+;;
+
 module Context_for_dune = struct
   type 'a monad = 'a Monad.t
   type filter = OpamTypes.filter
@@ -109,18 +113,32 @@ module Context_for_dune = struct
     | Unavailable -> Format.pp_print_string f "Availability condition not satisfied"
   ;;
 
-  let opam_version_compare =
-    let opam_file_compare_by_version =
-      let opam_package_version_compare a b =
-        OpamPackage.Version.compare a b |> Ordering.of_int
-      in
-      fun a b ->
-        opam_package_version_compare (OpamFile.OPAM.version a) (OpamFile.OPAM.version b)
+  (* Compare two packages where the "least" of the two packages is the
+     one that the solver should prefer. It is only meaningful to call
+     this function with two different versions of the same
+     package. This is sensitive to the configured version
+     preference. E.g. if the version preference is to prefer newer
+     packages then packages versions that are numerically greater will
+     be treated as less than versions that are numerically lower. This
+     comparison also accounts for the avoid-version flag by treating
+     any version with this flag set as greater than any version
+     without this flag so that the solver will prefer package versions
+     without this flag. *)
+  let opam_version_compare t a b =
+    let opam_version_compare a b = OpamPackage.Version.compare a b |> Ordering.of_int in
+    let ordering a b =
+      opam_version_compare (OpamFile.OPAM.version a) (OpamFile.OPAM.version b)
     in
-    fun t ->
+    let version_compare a b =
       match t.version_preference with
-      | Oldest -> opam_file_compare_by_version
-      | Newest -> Ordering.reverse opam_file_compare_by_version
+      | Oldest -> ordering a b
+      | Newest -> ordering b a
+    in
+    Tuple.T2.compare
+      Bool.compare
+      version_compare
+      (opam_file_is_avoid_version a, a)
+      (opam_file_is_avoid_version b, b)
   ;;
 
   let eval_to_bool (filter : filter) : (bool, [> `Not_a_bool of string ]) result =
@@ -181,10 +199,10 @@ module Context_for_dune = struct
     let+ resolved = Opam_repo.load_all_versions t.repos name in
     let available =
       OpamPackage.Version.Map.values resolved
-      (* This sort is not strictly necessary. The values returned from the map
-         are already sorted in ascending order. So it would be enough to just
-         reverse this list if we want the highest versions first. We leave the
-         sorting for clarity. *)
+      (* Note that although the packages are taken from a map,
+         explicitly sorting them is still necessary. This sort applies
+         the configured version preference and also allows the solver to
+         prefer versions without the avoid-version flag set. *)
       |> List.sort ~compare:(fun p1 p2 ->
         opam_version_compare
           t

test/blackbox-tests/test-cases/pkg/solve-compiler-dependency.t

@@ -40,13 +40,13 @@ compiler also in the repo.
   > flags: [avoid-version]
   > EOF
 
-The alpha version of the compiler was chosen instead of the stable version.
-This would ideally be avoided due to the avoid-version flag, but this flag is
-ignored by dune.
+The alpha version of the compiler is not chosen here because dune's
+solver respects the avoid-version flag between multiple versions of
+the same package.
   $ solve ocaml
   Solution for dune.lock:
   - ocaml.5.2.0
-  - ocaml-base-compiler.5.2.0+alpha1
+  - ocaml-base-compiler.5.2.0
 
 Now release a new version of ocaml-variants and a new version of ocaml that
 uses it. The dependency specification for ocaml is based on how the package is
@@ -61,9 +61,13 @@ organized in the wild.
   > ]
   > EOF
 
-Here ocaml-variants is chosen despite its avoid-version flag. This is because
-dune currently ignores this flag. This is a problem because the chosen compiler
-is not officially released and possibly unstable.
+Here ocaml-variants is chosen despite its avoid-version flag. This is
+because dune does not respect the avoid-version flag when choosing
+which package to use to satisfy a disjunction (the disjunction in
+question is between ocaml-base-compiler and ocaml-variants, where
+ocaml-variants has the avoid-version flag set and ocaml-base-compiler
+does not).  This is a problem because the chosen compiler is not
+officially released and possibly unstable.
   $ solve ocaml
   Solution for dune.lock:
   - ocaml.5.3.0