Include non-platform-specific solver vars in portable lockdirs (#12678)

Previously all expanded solver variables were omitted from lockdirs, as
some solver variables are specific to the platform being solved for.
This was too strict, and meant that non-platform-specific solver
variables like "with-doc" were incorrectly omitted. This change adds
non-platform-specific expanded solver variables to portable lockdirs.

Signed-off-by: Stephen Sherratt <[email protected]>

Author: gridbugs

src/dune_pkg/lock_dir.ml

@@ -1154,6 +1154,7 @@ let create_latest_version
       ~repos
       ~expanded_solver_variable_bindings
       ~solved_for_platform
+      ~portable_lock_dir
   =
   let packages =
     Package_name.Map.map packages ~f:(fun (pkg : Pkg.t) ->
@@ -1187,6 +1188,15 @@ let create_latest_version
   let solved_for_platform_platform_specific_only =
     Option.map solved_for_platform ~f:Solver_env.remove_all_except_platform_specific
   in
+  let expanded_solver_variable_bindings =
+    match portable_lock_dir with
+    | false -> expanded_solver_variable_bindings
+    | true ->
+      (* To make a portable lockdir, only include solver variables which are
+         not platform-specific. *)
+      Solver_stats.Expanded_variable_bindings.remove_platform_specific
+        expanded_solver_variable_bindings
+  in
   { version
   ; dependency_hash
   ; packages
@@ -1238,10 +1248,7 @@ let encode_metadata
      | None -> []
      | Some ocaml -> [ list sexp [ string "ocaml"; Package_name.encode (snd ocaml) ] ])
   @ [ list sexp (string "repositories" :: Repositories.encode repos) ]
-  @ (if
-       portable_lock_dir
-       || Solver_stats.Expanded_variable_bindings.is_empty
-            expanded_solver_variable_bindings
+  @ (if Solver_stats.Expanded_variable_bindings.is_empty expanded_solver_variable_bindings
      then []
      else
        [ list

src/dune_pkg/lock_dir.mli

@@ -126,7 +126,9 @@ val create_latest_version
   -> repos:Opam_repo.t list option
   -> expanded_solver_variable_bindings:Solver_stats.Expanded_variable_bindings.t
   -> solved_for_platform:Solver_env.t option
-       (* TODO: make this non-optional when portable lockdirs becomes the default *)
+       (* TODO: make the [solved_for_platform] argument non-optional when
+          portable lockdirs becomes the default *)
+  -> portable_lock_dir:bool
   -> t
 
 module Metadata : Dune_sexp.Versioned_file.S with type data := unit

src/dune_pkg/opam_solver.ml

@@ -1850,6 +1850,7 @@ let solve_lock_dir
            ~repos:(Some repos)
            ~expanded_solver_variable_bindings
            ~solved_for_platform:(Some solver_env)
+           ~portable_lock_dir
        in
        let+ files =
          match pkgs_by_name with

src/dune_pkg/solver_stats.ml

@@ -145,4 +145,15 @@ module Expanded_variable_bindings = struct
           ]
           ~hints))
   ;;
+
+  let remove_platform_specific { variable_values; unset_variables } =
+    let is_platform_specific variable_name =
+      Package_variable_name.Set.mem Package_variable_name.platform_specific variable_name
+    in
+    { variable_values =
+        List.filter variable_values ~f:(fun (variable_name, _) ->
+          not (is_platform_specific variable_name))
+    ; unset_variables = List.filter unset_variables ~f:(Fun.negate is_platform_specific)
+    }
+  ;;
 end

src/dune_pkg/solver_stats.mli

@@ -31,4 +31,7 @@ module Expanded_variable_bindings : sig
       common between [t] and [solver_env] is assigned the same value, and that
       all the unset variables in [t] are not assigned a value in [solver_env]. *)
   val validate_against_solver_env : t -> Solver_env.t -> unit
+
+  (** Remove all mention of platform-specific variables. *)
+  val remove_platform_specific : t -> t
 end

test/blackbox-tests/test-cases/pkg/portable-lockdirs/portable-lockdirs-with-doc.t

@@ -0,0 +1,51 @@
+Test that the with-doc variable is stored in the lockdir when it's set in
+dune-workspace.
+
+
+  $ . ../helpers.sh
+  $ mkrepo
+  $ add_mock_repo_if_needed
+
+  $ cat > dune-workspace <<EOF
+  > (lang dune 3.20)
+  > (pkg enabled)
+  > (lock_dir
+  >  (repositories mock)
+  >  (solver_env
+  >   (with-doc true)))
+  > (repository
+  >  (name mock)
+  >  (url "$PWD/mock-opam-repository"))
+  > EOF
+
+  $ cat > dune-project <<EOF
+  > (lang dune 3.18)
+  > (package
+  >  (name x)
+  >  (depends (foo :with-doc)))
+  > EOF
+
+  $ mkpkg foo
+
+  $ DUNE_CONFIG__PORTABLE_LOCK_DIR=enabled dune pkg lock
+  Solution for dune.lock
+  
+  This solution supports the following platforms:
+  - arch = x86_64; os = linux
+  - arch = arm64; os = linux
+  - arch = x86_64; os = macos
+  - arch = arm64; os = macos
+  
+  Dependencies on all supported platforms:
+  - foo.0.0.1
+
+The list-locked-dependencies command does some validation that there are no
+extraneous packages in the lockdir. It uses the solver variables stored in the
+lockdir when filtering dependencies which have predicates such as ":with-doc".
+If the with-doc variable wasn't stored in the lockdir then this command would
+fail as the locked dependency "foo" would appear extraneous.
+  $ dune describe pkg list-locked-dependencies
+  Dependencies of local packages locked in dune.lock
+  - Immediate dependencies of local package x.dev
+    - foo.0.0.1
+    

test/expect-tests/dune_pkg/dune_pkg_unit_tests.ml

@@ -114,7 +114,8 @@ let%expect_test "encode/decode round trip test for lockdir with no deps" =
          ~ocaml:None
          ~repos:None
          ~expanded_solver_variable_bindings:Expanded_variable_bindings.empty
-         ~solved_for_platform:None)
+         ~solved_for_platform:None
+         ~portable_lock_dir:false)
     ();
   [%expect
     {|
@@ -167,6 +168,7 @@ let%expect_test "encode/decode round trip test for lockdir with simple deps" =
            ; unset_variables = [ Package_variable_name.os_family ]
            }
          ~solved_for_platform:None
+         ~portable_lock_dir:false
          (Package_name.Map.of_list_exn
             [ mk_pkg_basic ~name:"foo" ~version:(Package_version.of_string "0.1.0")
             ; mk_pkg_basic ~name:"bar" ~version:(Package_version.of_string "0.2.0")
@@ -328,6 +330,7 @@ let%expect_test "encode/decode round trip test for lockdir with complex deps" =
       ~repos:(Some [ opam_repo ])
       ~expanded_solver_variable_bindings:Expanded_variable_bindings.empty
       ~solved_for_platform:None
+      ~portable_lock_dir:false
       (Package_name.Map.of_list_exn [ pkg_a; pkg_b; pkg_c ])
   in
   lock_dir_encode_decode_round_trip_test ~lock_dir_path:"complex_lock_dir" ~lock_dir ();
@@ -475,6 +478,7 @@ let%expect_test "encode/decode round trip test with locked repo revision" =
         ~repos:(Some [ opam_repo ])
         ~expanded_solver_variable_bindings:Expanded_variable_bindings.empty
         ~solved_for_platform:None
+        ~portable_lock_dir:false
         (Package_name.Map.of_list_exn [ pkg_a; pkg_b; pkg_c ])
     in
     lock_dir_encode_decode_round_trip_test