From 9a8b3c82b74e48f42ac94a0b4beec78c04aa4c6c Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Sun, 3 Nov 2024 21:05:45 +0000 Subject: [PATCH 1/4] test(pkg): reproduce #11058 Signed-off-by: Rudi Grinberg --- .../test-cases/pkg/depopts/gh11058.t | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 test/blackbox-tests/test-cases/pkg/depopts/gh11058.t diff --git a/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t b/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t new file mode 100644 index 00000000000..f83d913f1ea --- /dev/null +++ b/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t @@ -0,0 +1,43 @@ +Reproduce github issue #11058 + +Handling of more than one depopt: + + $ . ../helpers.sh + + $ mkpkg bar <<'EOF' + > depopts: [ "a" "b" "c" ] + > EOF + + $ solve bar + Internal error, please report upstream including the contents of _build/log. + Description: + ("invalid depopts", + { depopts = + Or [ Or [ Atom ("a", Empty); Atom ("b", Empty) ]; Atom ("c", Empty) ] + }) + Raised at Stdune__Code_error.raise in file + "otherlibs/stdune/src/code_error.ml", line 10, characters 30-62 + Called from Dune_pkg__Opam_solver.resolve_depopts in file + "src/dune_pkg/opam_solver.ml", line 565, characters 3-21 + Called from Dune_pkg__Opam_solver.opam_package_to_lock_file_pkg in file + "src/dune_pkg/opam_solver.ml", line 645, characters 6-48 + Called from Stdlib__List.rev_map.rmap_f in file "list.ml" (inlined), line + 105, characters 22-25 + Called from Stdlib__List.rev_map in file "list.ml", line 107, characters 2-13 + Called from Stdune__List.map in file "otherlibs/stdune/src/list.ml", line 5, + characters 19-33 + Called from Dune_pkg__Opam_solver.solve_lock_dir.(fun) in file + "src/dune_pkg/opam_solver.ml", lines 889-896, characters 8-30 + Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml", + line 79, characters 8-11 + Re-raised at Stdune__Exn.raise_with_backtrace in file + "otherlibs/stdune/src/exn.ml", line 38, characters 27-56 + Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml", + line 79, characters 8-11 + + I must not crash. Uncertainty is the mind-killer. Exceptions are the + little-death that brings total obliteration. I will fully express my cases. + Execution will pass over me and through me. And when it has gone past, I + will unwind the stack along its path. Where the cases are handled there will + be nothing. Only I will remain. + [1] From 9190d09dbdf53214fef153addf54958d49f8f1b0 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Sun, 3 Nov 2024 22:37:25 +0000 Subject: [PATCH 2/4] fix(pkg): handle nested [Or] in depopts Flatten expressions such as [Or (Or .., ..))] before processing depopts Signed-off-by: Rudi Grinberg Signed-off-by: Rudi Grinberg --- src/dune_pkg/opam_solver.ml | 40 ++++++++++--------- .../test-cases/pkg/depopts/gh11058.t | 34 +--------------- 2 files changed, 23 insertions(+), 51 deletions(-) diff --git a/src/dune_pkg/opam_solver.ml b/src/dune_pkg/opam_solver.ml index ea1c894c9bc..0c68ded8a71 100644 --- a/src/dune_pkg/opam_solver.ml +++ b/src/dune_pkg/opam_solver.ml @@ -551,25 +551,27 @@ let opam_file_is_compiler (opam_package : OpamFile.OPAM.t) = ;; let resolve_depopts ~resolve depopts = - (let rec collect acc depopts = - match (depopts : OpamTypes.filtered_formula) with - | Or ((Atom (_, _) as dep), depopts) -> collect (dep :: acc) depopts - | Atom (_, _) -> depopts :: acc - | Empty -> acc - | _ -> - (* We rely on depopts always being a list of or'ed package names. Opam - verifies this for us at parsing time. Dune projects have this - restriction for depopts and regular deps *) - Code_error.raise "invalid depopts" [ "depopts", Opam_dyn.filtered_formula depopts ] - in - collect [] depopts) - |> List.rev - |> List.concat_map ~f:(fun depopt -> - match resolve depopt with - | Error _ -> [] - | Ok { Resolve_opam_formula.post = _; regular } -> - (* CR-someday rgrinberg: think about post deps *) - regular) + let rec collect acc depopts = + match (depopts : OpamTypes.filtered_formula) with + | Or ((Atom (_, _) as dep), depopts) -> collect (dep :: acc) depopts + | Atom (_, _) as dep -> dep :: acc + | Empty -> acc + | _ -> + (* We rely on depopts always being a list of or'ed package names. Opam + verifies this for us at parsing time. Dune projects have this + restriction for depopts and regular deps *) + Code_error.raise "invalid depopts" [ "depopts", Opam_dyn.filtered_formula depopts ] + in + OpamFormula.ors_to_list depopts + |> List.concat_map ~f:(fun x -> + collect [] x + |> List.rev + |> List.concat_map ~f:(fun depopt -> + match resolve depopt with + | Error _ -> [] + | Ok { Resolve_opam_formula.post = _; regular } -> + (* CR-someday rgrinberg: think about post deps *) + regular)) ;; let opam_package_to_lock_file_pkg diff --git a/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t b/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t index f83d913f1ea..3dfb49991d5 100644 --- a/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t +++ b/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t @@ -9,35 +9,5 @@ Handling of more than one depopt: > EOF $ solve bar - Internal error, please report upstream including the contents of _build/log. - Description: - ("invalid depopts", - { depopts = - Or [ Or [ Atom ("a", Empty); Atom ("b", Empty) ]; Atom ("c", Empty) ] - }) - Raised at Stdune__Code_error.raise in file - "otherlibs/stdune/src/code_error.ml", line 10, characters 30-62 - Called from Dune_pkg__Opam_solver.resolve_depopts in file - "src/dune_pkg/opam_solver.ml", line 565, characters 3-21 - Called from Dune_pkg__Opam_solver.opam_package_to_lock_file_pkg in file - "src/dune_pkg/opam_solver.ml", line 645, characters 6-48 - Called from Stdlib__List.rev_map.rmap_f in file "list.ml" (inlined), line - 105, characters 22-25 - Called from Stdlib__List.rev_map in file "list.ml", line 107, characters 2-13 - Called from Stdune__List.map in file "otherlibs/stdune/src/list.ml", line 5, - characters 19-33 - Called from Dune_pkg__Opam_solver.solve_lock_dir.(fun) in file - "src/dune_pkg/opam_solver.ml", lines 889-896, characters 8-30 - Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml", - line 79, characters 8-11 - Re-raised at Stdune__Exn.raise_with_backtrace in file - "otherlibs/stdune/src/exn.ml", line 38, characters 27-56 - Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml", - line 79, characters 8-11 - - I must not crash. Uncertainty is the mind-killer. Exceptions are the - little-death that brings total obliteration. I will fully express my cases. - Execution will pass over me and through me. And when it has gone past, I - will unwind the stack along its path. Where the cases are handled there will - be nothing. Only I will remain. - [1] + Solution for dune.lock: + - bar.0.0.1 From afd57da5867abc822199eb044d2e28597f65b7a0 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Mon, 4 Nov 2024 22:34:20 +0000 Subject: [PATCH 3/4] chore: clarify comment Signed-off-by: Rudi Grinberg Signed-off-by: Rudi Grinberg --- src/dune_pkg/opam_solver.ml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dune_pkg/opam_solver.ml b/src/dune_pkg/opam_solver.ml index 0c68ded8a71..0ab95c1d08e 100644 --- a/src/dune_pkg/opam_solver.ml +++ b/src/dune_pkg/opam_solver.ml @@ -558,8 +558,8 @@ let resolve_depopts ~resolve depopts = | Empty -> acc | _ -> (* We rely on depopts always being a list of or'ed package names. Opam - verifies this for us at parsing time. Dune projects have this - restriction for depopts and regular deps *) + verifies this for us at parsing time. Packages defined in dune-project + files have this restriction for depopts and regular deps *) Code_error.raise "invalid depopts" [ "depopts", Opam_dyn.filtered_formula depopts ] in OpamFormula.ors_to_list depopts From 04b9b39647fcf5e574ac28d72df46d45127807a3 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Mon, 4 Nov 2024 22:38:06 +0000 Subject: [PATCH 4/4] test(pkg): add more disjunction pattern cases Signed-off-by: Rudi Grinberg Signed-off-by: Rudi Grinberg --- .../test-cases/pkg/depopts/gh11058.t | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t b/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t index 3dfb49991d5..b5fc51beb93 100644 --- a/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t +++ b/test/blackbox-tests/test-cases/pkg/depopts/gh11058.t @@ -4,10 +4,40 @@ Handling of more than one depopt: $ . ../helpers.sh - $ mkpkg bar <<'EOF' + $ mkpkg a + $ mkpkg b + $ mkpkg c + $ mkpkg d + $ mkpkg e + $ mkpkg f + + $ runtest() { + > mkpkg bar <<'EOF' + > depopts: [ "a" "b" "c" ] + > EOF + > solve bar + > } + + $ runtest <<'EOF' > depopts: [ "a" "b" "c" ] > EOF + Solution for dune.lock: + - bar.0.0.1 - $ solve bar + $ runtest <<'EOF' + > depopts: [ "a" "b" "c" "d" ] + > EOF + Solution for dune.lock: + - bar.0.0.1 + + $ runtest <<'EOF' + > depopts: [ ("a" | "b") "c" "d" ] + > EOF + Solution for dune.lock: + - bar.0.0.1 + + $ runtest <<'EOF' + > depopts: [ (("e" | "a") | ("d" | "f")) "b" "c" ] + > EOF Solution for dune.lock: - bar.0.0.1