Don't run solver when direct dep doesn't build with dune

This is a workaround for
tarides#385. The error message
displayed when a dependency doesn't build with dune is misleading in
some cases. This adds a check that direct dependencies all have
available versions which build with dune before running the solver.

CHANGES.md

@@ -7,6 +7,8 @@
 - Add option `--keep-symlinked-dir` to preserve symlinks in `duniverse/`, which
   can be useful for local development. (#348, #366, @hannesm,
   @Leonidas-from-XIV)
+- Don't run the solver when a direct dependency doesn't depend on dune (#385,
+  #386, @gridbugs)
 
 ### Changed
 

cli/lock.ml

@@ -339,6 +339,64 @@ let extract_opam_env ~source_config global_state =
   | { global_vars = Some env; _ } -> env
   | { global_vars = None; _ } -> opam_env_from_global_state global_state
 
+(* In some cases there will be no solution to the dependencies of a package
+   due to a dependency not building with dune but the solver will omit this
+   fact from its diagnostic information leading to misleading error messages
+   (see https://github.com/tarides/opam-monorepo/issues/385). To avoid this
+   issue in some (but not all!) cases, this function checks the direct
+   dependencies of the local opam files and results in an error if any
+   dependency has no version available in the current switch which builds
+   with dune. If this is found to be the case we can avoid invoking the
+   solver at all. Note that this false negatives; it won't detect when a
+   transitive dependency of an opam file doesn't build with dune. This is
+   deliberate as handling these cases in general would be akin to
+   implementing a solver. This is intended as a temporary workaround of
+   https://github.com/tarides/opam-monorepo/issues/385. *)
+let check_that_direct_dependencies_are_valid_dune_wise ~local_opam_files
+    ~switch_state ~allow_jbuilder =
+  let all_packages =
+    Lazy.force switch_state.OpamStateTypes.available_packages
+  in
+  let is_package_valid_dune_wise package =
+    let opam_file = OpamSwitchState.opam switch_state package in
+    D.Opam_solve.is_valid_dune_wise opam_file ~allow_jbuilder
+  in
+  let dep_candidates_by_name_of_opam opam_file =
+    let dep_formula =
+      OpamFile.OPAM.depends opam_file
+      |> OpamFilter.filter_deps ~build:true ~post:true ~test:false ~doc:false
+           ~dev:false
+    in
+    let dep_candidates = OpamFormula.packages all_packages dep_formula in
+    OpamPackage.Set.fold
+      (fun package map ->
+        let name = OpamPackage.name package in
+        OpamPackage.Name.Map.update name
+          (OpamPackage.Set.add package)
+          OpamPackage.Set.empty map)
+      dep_candidates OpamPackage.Name.Map.empty
+  in
+  let dep_candidate_names_with_no_dune_versions_of_opam opam_file =
+    dep_candidates_by_name_of_opam opam_file
+    |> OpamPackage.Name.Map.filter (fun _ packages ->
+           OpamPackage.Set.is_empty
+             (OpamPackage.Set.filter is_package_valid_dune_wise packages))
+    |> OpamPackage.Name.Map.keys
+  in
+  let direct_dependencies_that_don't_build_with_dune =
+    OpamPackage.Name.Map.values local_opam_files
+    |> List.concat_map ~f:(fun (_, opam_file) ->
+           dep_candidate_names_with_no_dune_versions_of_opam opam_file)
+    |> OpamPackage.Name.Set.of_list |> OpamPackage.Name.Set.elements
+  in
+  if List.length direct_dependencies_that_don't_build_with_dune == 0 then Ok ()
+  else
+    let repositories = current_repos ~switch_state in
+    Error
+      (`Msg
+        (error_message_when_dependencies_don't_build_with_dune ~repositories
+           direct_dependencies_that_don't_build_with_dune))
+
 let calculate_opam ~source_config ~build_only ~allow_jbuilder
     ~require_cross_compile ~preferred_versions ~local_opam_files ~ocaml_version
     ~target_packages =
@@ -384,6 +442,10 @@ let calculate_opam ~source_config ~build_only ~allow_jbuilder
               Logs.info (fun l ->
                   l "Solve using current opam switch: %s"
                     (OpamSwitch.to_string switch_state.switch));
+              let* () =
+                check_that_direct_dependencies_are_valid_dune_wise
+                  ~local_opam_files ~switch_state ~allow_jbuilder
+              in
               let solver = D.Opam_solve.local_opam_config_solver in
               let dependency_entries =
                 D.Opam_solve.calculate ~build_only ~allow_jbuilder

lib/opam_solve.ml

@@ -38,6 +38,17 @@ module type OPAM_MONOREPO_CONTEXT = sig
     Takes into account local packages an pin-depends. *)
 end
 
+let is_valid_dune_wise opam_file ~allow_jbuilder =
+  let pkg = OpamFile.OPAM.package opam_file in
+  let depends = OpamFile.OPAM.depends opam_file in
+  let depopts = OpamFile.OPAM.depopts opam_file in
+  let uses_dune =
+    Opam.depends_on_dune ~allow_jbuilder depends
+    || Opam.depends_on_dune ~allow_jbuilder depopts
+  in
+  let summary = Opam.Package_summary.from_opam pkg opam_file in
+  Opam.Package_summary.is_safe_package summary || uses_dune
+
 module Opam_monorepo_context (Base_context : BASE_CONTEXT) :
   OPAM_MONOREPO_CONTEXT
     with type base_rejection = Base_context.rejection
@@ -85,22 +96,12 @@ module Opam_monorepo_context (Base_context : BASE_CONTEXT) :
       preferred_versions;
     }
 
-  let validate_candidate ~allow_jbuilder ~must_cross_compile ~require_dune ~name
-      ~version opam_file =
+  let validate_candidate ~allow_jbuilder ~must_cross_compile ~require_dune
+      opam_file =
     (* this function gets called way too often.. memoize? *)
-    let pkg = OpamPackage.create name version in
-    let depends = OpamFile.OPAM.depends opam_file in
-    let depopts = OpamFile.OPAM.depopts opam_file in
-    let uses_dune =
-      Opam.depends_on_dune ~allow_jbuilder depends
-      || Opam.depends_on_dune ~allow_jbuilder depopts
-    in
-    let summary = Opam.Package_summary.from_opam pkg opam_file in
-    let is_valid_dune_wise =
-      Opam.Package_summary.is_safe_package summary
-      || (not require_dune) || uses_dune
-    in
-    match is_valid_dune_wise with
+    match
+      (not require_dune) || is_valid_dune_wise opam_file ~allow_jbuilder
+    with
     | false -> Error Non_dune
     | true when (not must_cross_compile) || Opam.has_cross_compile_tag opam_file
       ->
@@ -139,7 +140,7 @@ module Opam_monorepo_context (Base_context : BASE_CONTEXT) :
     let depends = remove_opam_provided_from_formula opam_provided depends in
     OpamFile.OPAM.with_depends depends opam_file
 
-  let filter_candidates ~allow_jbuilder ~must_cross_compile ~require_dune ~name
+  let filter_candidates ~allow_jbuilder ~must_cross_compile ~require_dune
       versions =
     List.map
       ~f:(fun (version, result) ->
@@ -148,7 +149,7 @@ module Opam_monorepo_context (Base_context : BASE_CONTEXT) :
         | Ok opam_file ->
             let res =
               validate_candidate ~allow_jbuilder ~must_cross_compile
-                ~require_dune ~name ~version opam_file
+                ~require_dune opam_file
             in
             (version, res))
       versions
@@ -222,7 +223,7 @@ module Opam_monorepo_context (Base_context : BASE_CONTEXT) :
           OpamPackage.Name.Map.find_opt name preferred_versions
         in
         filter_candidates ~allow_jbuilder ~must_cross_compile ~require_dune
-          ~name candidates
+          candidates
         |> remove_opam_provided ~opam_provided
         |> demote_candidates_to_avoid
         |> promote_version preferred_version

lib/opam_solve.mli

@@ -12,6 +12,8 @@ val local_opam_config_solver : (switch, switch_diagnostics) t
 val explicit_repos_solver :
   (opam_env * explicit_repos, explicit_repos_diagnostics) t
 
+val is_valid_dune_wise : OpamFile.OPAM.t -> allow_jbuilder:bool -> bool
+
 val calculate :
   build_only:bool ->
   allow_jbuilder:bool ->