Include non-platform-specific solver vars in portable lockdirs

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

@@ -127,6 +127,7 @@ val create_latest_version
   -> 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 *)
+  -> portable_lock_dir:bool
   -> t
 
 module Metadata : Dune_sexp.Versioned_file.S with type data := unit

src/dune_pkg/opam_solver.ml

@@ -1838,6 +1838,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