From 035cff00cfca26466ea0cdfa0ac2b29084a58118 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Fri, 19 Jul 2024 23:14:11 +0100 Subject: [PATCH] feature(pkg): set [post = true] when solving (#10752) Signed-off-by: Rudi Grinberg --- src/dune_pkg/opam_solver.ml | 2 +- src/dune_pkg/package_universe.ml | 2 +- src/dune_pkg/resolve_opam_formula.ml | 63 +++++++++++++- src/dune_pkg/resolve_opam_formula.mli | 10 ++- src/dune_pkg/solver_env.ml | 1 + .../test-cases/pkg/common-filters-deps.t | 1 + .../pkg/compiler-post-dependencies.t | 54 ++++++++++++ test/blackbox-tests/test-cases/pkg/gh10670.t | 44 ---------- .../test-cases/pkg/just-print-solver-env.t | 4 + .../test-cases/pkg/post-deps-solving.t | 86 +++++++++++++++++++ 10 files changed, 218 insertions(+), 49 deletions(-) create mode 100644 test/blackbox-tests/test-cases/pkg/compiler-post-dependencies.t delete mode 100644 test/blackbox-tests/test-cases/pkg/gh10670.t create mode 100644 test/blackbox-tests/test-cases/pkg/post-deps-solving.t diff --git a/src/dune_pkg/opam_solver.ml b/src/dune_pkg/opam_solver.ml index 68262e818c3..7167cab2934 100644 --- a/src/dune_pkg/opam_solver.ml +++ b/src/dune_pkg/opam_solver.ml @@ -636,7 +636,7 @@ let opam_package_to_lock_file_pkg version_by_package_name opam_file.depends with - | Ok dep_package_names -> dep_package_names + | Ok { regular; _ } -> regular | Error (`Formula_could_not_be_satisfied hints) -> Code_error.raise "Dependencies of package can't be satisfied from packages in solution" diff --git a/src/dune_pkg/package_universe.ml b/src/dune_pkg/package_universe.ml index fc74f8c0c4d..74a7005a50f 100644 --- a/src/dune_pkg/package_universe.ml +++ b/src/dune_pkg/package_universe.ml @@ -55,7 +55,7 @@ let concrete_dependencies_of_local_package t local_package_name ~with_test = (Solver_env.to_env t.solver_env) t.version_by_package_name with - | Ok s -> s + | Ok { regular; post = _ } -> regular | Error (`Formula_could_not_be_satisfied unsatisfied_formula_hints) -> User_error.raise ?hints:(Option.some_if with_test lockdir_regenerate_hints) diff --git a/src/dune_pkg/resolve_opam_formula.ml b/src/dune_pkg/resolve_opam_formula.ml index 88cea7a4e89..e4d00d6baea 100644 --- a/src/dune_pkg/resolve_opam_formula.ml +++ b/src/dune_pkg/resolve_opam_formula.ml @@ -154,6 +154,67 @@ let formula_to_package_names version_by_package_name opam_formula = Ok package_names ;; +(* Override the setting of the "post" variable to a given boolean + value by passing [Some value], or force it to be unset by passing + [None]. *) +let override_post post_value env var = + match OpamVariable.Full.scope var with + | Global + when Package_variable_name.equal + (Package_variable_name.of_opam @@ OpamVariable.Full.variable var) + Package_variable_name.post -> Option.map post_value ~f:(fun b -> OpamTypes.B b) + | _ -> env var +;; + +(* Check that a package version satisfies the version constraint + associated with a package dependency in an opam file. *) +let package_version_satisfies_opam_version_constraint_opt + package_version + opam_version_constraint_opt + = + match opam_version_constraint_opt with + | None -> true + | Some (constraint_ : OpamFormula.version_constraint) -> + let opam_version = Package_version.to_opam_package_version package_version in + let version_formula = OpamFormula.Atom constraint_ in + OpamFormula.check_version_formula version_formula opam_version +;; + +(* TODO (steve): This does a very similar thing to + [formula_to_package_names] so the two functions should be + combined. *) +let formula_to_package_names_allow_missing version_by_package_name opam_formula = + let cnf_terms = OpamFormula.to_cnf opam_formula in + List.filter_map cnf_terms ~f:(fun disjunction -> + (* Take the first term of the disjunction that is part of the set of packages in the + solution, if any. *) + List.find_map disjunction ~f:(fun (opam_package_name, version_constraint_opt) -> + let package_name = Package_name.of_opam_package_name opam_package_name in + Package_name.Map.find version_by_package_name package_name + |> Option.bind ~f:(fun version_in_solution -> + if package_version_satisfies_opam_version_constraint_opt + version_in_solution + version_constraint_opt + then Some package_name + else None))) +;; + +type deps = + { post : Package_name.t list + ; regular : Package_name.t list + } + let filtered_formula_to_package_names env ~with_test version_by_package_name formula = - formula_to_package_names version_by_package_name (apply_filter ~with_test env formula) + let open Result.O in + let+ all = + formula_to_package_names version_by_package_name (apply_filter ~with_test env formula) + in + let regular_set = + formula_to_package_names_allow_missing + version_by_package_name + (apply_filter ~with_test (override_post (Some false) env) formula) + |> Package_name.Set.of_list + in + let regular, post = List.partition all ~f:(Package_name.Set.mem regular_set) in + { regular; post } ;; diff --git a/src/dune_pkg/resolve_opam_formula.mli b/src/dune_pkg/resolve_opam_formula.mli index 662069cd24a..1abfbbf271c 100644 --- a/src/dune_pkg/resolve_opam_formula.mli +++ b/src/dune_pkg/resolve_opam_formula.mli @@ -44,12 +44,18 @@ val formula_to_package_names -> OpamTypes.formula -> (Package_name.t list, unsatisfied_formula) result +type deps = + { post : Package_name.t list + ; regular : Package_name.t list + } + (** Like [formula_to_package_name] but takes an [OpamTypes.filtered_formula] and evaluates its filters to produce a formula which is then resolved to a - list of package names. *) + list of package names, split into post and regular (ie. non-post) + dependencies. *) val filtered_formula_to_package_names : OpamFilter.env -> with_test:bool -> Package_version.t Package_name.Map.t -> OpamTypes.filtered_formula - -> (Package_name.t list, unsatisfied_formula) result + -> (deps, unsatisfied_formula) result diff --git a/src/dune_pkg/solver_env.ml b/src/dune_pkg/solver_env.ml index 3cbc005b6e0..ff06e563b3d 100644 --- a/src/dune_pkg/solver_env.ml +++ b/src/dune_pkg/solver_env.ml @@ -54,6 +54,7 @@ let with_defaults = , OpamVersion.to_string OpamVersion.current |> Variable_value.string ) ; Package_variable_name.with_doc, Variable_value.false_ ; Package_variable_name.with_dev_setup, Variable_value.false_ + ; Package_variable_name.post, Variable_value.true_ ] |> List.fold_left ~init:empty ~f:(fun acc (name, value) -> set acc name value) ;; diff --git a/test/blackbox-tests/test-cases/pkg/common-filters-deps.t b/test/blackbox-tests/test-cases/pkg/common-filters-deps.t index eb2f5b619be..2ab9586210e 100644 --- a/test/blackbox-tests/test-cases/pkg/common-filters-deps.t +++ b/test/blackbox-tests/test-cases/pkg/common-filters-deps.t @@ -22,4 +22,5 @@ documentation-only deps are omitted from the solution. $ solve "(test :with-test) (doc :with-doc) (dev-setup :with-dev-setup) (dev :with-dev) (build :build) (post :post)" Solution for dune.lock: - build.0.0.1 + - post.0.0.1 - test.0.0.1 diff --git a/test/blackbox-tests/test-cases/pkg/compiler-post-dependencies.t b/test/blackbox-tests/test-cases/pkg/compiler-post-dependencies.t new file mode 100644 index 00000000000..83c4091c259 --- /dev/null +++ b/test/blackbox-tests/test-cases/pkg/compiler-post-dependencies.t @@ -0,0 +1,54 @@ +Exercise dune resolving the post dependencies found in compiler packages. + + $ . ./helpers.sh + $ mkrepo + + $ cat >dune-workspace << EOF + > (lang dune 3.16) + > (lock_dir + > (path dune.lock) + > (repositories mock) + > (solver_env + > (os linux))) + > EOF + + $ mkpkg host-system-other + + $ mkpkg system-mingw << EOF + > available: os = "win32" + > EOF + + $ mkpkg ocaml << EOF + > depends: [ + > "ocaml-base-compiler" + > ] + > EOF + +This package has a dependency cycle with the "ocaml" package, broken +by the use of post dependencies. It also contains a formula with no +solutions when the "post" variable is set to "false" on non-windows +systems. Opam evaluates dependency formulae with post=true and then +does further filtering to remove post dependencies from dependency +lists to prevent circular dependencies at package build time. + $ mkpkg ocaml-base-compiler << EOF + > depends: [ + > "ocaml" {post} + > (("arch-x86_64" {os = "win32" & arch = "x86_64"} & "system-mingw" & + > "mingw-w64-shims" {os-distribution = "cygwin" & build}) | + > ("arch-x86_32" {os = "win32"} & "ocaml-option-bytecode-only" & + > "system-mingw" & + > "mingw-w64-shims" {os-distribution = "cygwin" & build}) | + > "host-system-other" {os != "win32" & post}) + > ] + > EOF + + $ solve ocaml-base-compiler + Solution for dune.lock: + - host-system-other.0.0.1 + - ocaml.0.0.1 + - ocaml-base-compiler.0.0.1 + +Ensure that packages can be resolved at build time. This checks that +the dependency cycle between the "ocaml" and "ocaml-base-compiler" +packages is successfully broken by the use of post dependencies. + $ dune build diff --git a/test/blackbox-tests/test-cases/pkg/gh10670.t b/test/blackbox-tests/test-cases/pkg/gh10670.t deleted file mode 100644 index 4f1946095b1..00000000000 --- a/test/blackbox-tests/test-cases/pkg/gh10670.t +++ /dev/null @@ -1,44 +0,0 @@ -Reproduces https://github.com/ocaml/dune/issues/10670 - -Dune solves packages with "post" set to false, so the disjunction in -ocaml-base-compiler's dependencies can't be resolved on non-windows -systems. - - $ . ./helpers.sh - $ mkrepo - - $ cat >dune-workspace << EOF - > (lang dune 3.16) - > (lock_dir - > (path dune.lock) - > (repositories mock) - > (solver_env - > (os linux))) - > EOF - - $ mkpkg host-system-other - - $ mkpkg system-mingw << EOF - > available: os = "win32" - > EOF - - $ mkpkg ocaml-base-compiler << EOF - > depends: [ - > (("arch-x86_64" {os = "win32" & arch = "x86_64"} & "system-mingw" & - > "mingw-w64-shims" {os-distribution = "cygwin" & build}) | - > ("arch-x86_32" {os = "win32"} & "ocaml-option-bytecode-only" & - > "system-mingw" & - > "mingw-w64-shims" {os-distribution = "cygwin" & build}) | - > "host-system-other" {os != "win32" & post}) - > ] - > EOF - - $ solve ocaml-base-compiler - Error: Unable to solve dependencies for the following lock directories: - Lock directory dune.lock: - Can't find all required versions. - Selected: ocaml-base-compiler.0.0.1 x.dev system-mingw - - system-mingw -> (problem) - No usable implementations: - system-mingw.0.0.1: Availability condition not satisfied - [1] diff --git a/test/blackbox-tests/test-cases/pkg/just-print-solver-env.t b/test/blackbox-tests/test-cases/pkg/just-print-solver-env.t index 1d884e660dc..d00141a7077 100644 --- a/test/blackbox-tests/test-cases/pkg/just-print-solver-env.t +++ b/test/blackbox-tests/test-cases/pkg/just-print-solver-env.t @@ -40,11 +40,13 @@ Add some build contexts with different environments $ dune pkg print-solver-env --all Solver environment for lock directory change-opam-version.lock: - opam-version = 42 + - post = true - with-dev-setup = false - with-doc = false Solver environment for lock directory dune.linux.lock: - opam-version = 2.2.0~alpha-vendored - os = linux + - post = true - with-dev-setup = false - with-doc = false Solver environment for lock directory dune.linux.no-doc.lock: @@ -54,10 +56,12 @@ Add some build contexts with different environments - os-distribution = ubuntu - os-family = ubuntu - os-version = 22.04 + - post = true - sys-ocaml-version = 5.0 - with-dev-setup = false - with-doc = false Solver environment for lock directory dune.lock: - opam-version = 2.2.0~alpha-vendored + - post = true - with-dev-setup = false - with-doc = false diff --git a/test/blackbox-tests/test-cases/pkg/post-deps-solving.t b/test/blackbox-tests/test-cases/pkg/post-deps-solving.t new file mode 100644 index 00000000000..865ea47872f --- /dev/null +++ b/test/blackbox-tests/test-cases/pkg/post-deps-solving.t @@ -0,0 +1,86 @@ +Solving for post dependencies: + + $ . ./helpers.sh + $ mkrepo + + $ mkpkg bar + + $ mkpkg foo < depends: [ "bar" {post} ] + > EOF + +We don't need bar, but we still include it: + + $ solve foo + Solution for dune.lock: + - bar.0.0.1 + - foo.0.0.1 + + $ cat dune.lock/foo.pkg dune.lock/bar.pkg + (version 0.0.1) + (version 0.0.1) + +Self dependency + + $ mkpkg foo < depends: [ "foo" {post} ] + > EOF + + $ solve foo + Solution for dune.lock: + - foo.0.0.1 + + $ cat dune.lock/foo.pkg + (version 0.0.1) + +Using post to break cycle: + + $ mkpkg foo < depends: [ "bar" {post} ] + > EOF + + $ mkpkg bar < depends: [ "foo" ] + > EOF + + $ solve bar + Solution for dune.lock: + - bar.0.0.1 + - foo.0.0.1 + + $ cat dune.lock/foo.pkg dune.lock/bar.pkg + (version 0.0.1) + (version 0.0.1) + + (depends foo) + +post "cycle": + + $ mkpkg foo < depends: [ "bar" {post} ] + > EOF + + $ mkpkg bar < depends: [ "foo" {post} ] + > EOF + + $ solve foo + Solution for dune.lock: + - bar.0.0.1 + - foo.0.0.1 + + $ cat dune.lock/foo.pkg dune.lock/bar.pkg + (version 0.0.1) + (version 0.0.1) + +In depopts: + + $ mkpkg foo < depopts: [ "bar" {post} ] + > EOF + + $ mkpkg bar + + $ solve foo + Solution for dune.lock: + - foo.0.0.1