From 3e25617120a4a70031337e10ade0cef95855cc3b Mon Sep 17 00:00:00 2001 From: Stephen Sherratt Date: Fri, 28 Apr 2023 17:12:31 +1000 Subject: [PATCH] Don't run solver when direct dep doesn't build with dune This is a workaround for https://github.com/tarides/opam-monorepo/issues/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 | 2 ++ cli/lock.ml | 62 ++++++++++++++++++++++++++++++++++++++++++++++ lib/opam_solve.ml | 37 +++++++++++++-------------- lib/opam_solve.mli | 2 ++ 4 files changed, 85 insertions(+), 18 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 23d972ee2..7a2987beb 100644 --- a/CHANGES.md +++ b/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 diff --git a/cli/lock.ml b/cli/lock.ml index 15a8c2320..364d70252 100644 --- a/cli/lock.ml +++ b/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 diff --git a/lib/opam_solve.ml b/lib/opam_solve.ml index 18aff4b4c..9ce5b60de 100644 --- a/lib/opam_solve.ml +++ b/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 diff --git a/lib/opam_solve.mli b/lib/opam_solve.mli index 79681d842..8705620cf 100644 --- a/lib/opam_solve.mli +++ b/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 ->