-
Notifications
You must be signed in to change notification settings - Fork 429
Experimental portable lockdirs #11489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
open Dune_config | ||
open Import | ||
open Pkg_common | ||
module Package_version = Dune_pkg.Package_version | ||
|
@@ -66,11 +67,75 @@ let resolve_project_pins project_pins = | |
Pin_stanza.resolve project_pins ~scan_project | ||
;; | ||
|
||
let solve_multiple_envs | ||
base_solver_env | ||
version_preference | ||
repos | ||
~pins | ||
~local_packages | ||
~constraints | ||
= | ||
let open Fiber.O in | ||
let solve_for_env env = | ||
Dune_pkg.Opam_solver.solve_lock_dir | ||
env | ||
version_preference | ||
repos | ||
~pins | ||
~local_packages | ||
~constraints | ||
in | ||
let portable_solver_env = | ||
(* TODO: make sure nothing system-specific sneaks into the environment here *) | ||
Dune_pkg.Solver_env.unset_multi | ||
base_solver_env | ||
Dune_lang.Package_variable_name.platform_specific | ||
in | ||
let+ results = | ||
Fiber.parallel_map Dune_pkg.Solver_env.popular_platform_envs ~f:(fun platform_env -> | ||
let solver_env = Dune_pkg.Solver_env.extend portable_solver_env platform_env in | ||
solve_for_env solver_env) | ||
in | ||
let results, errors = | ||
List.partition_map results ~f:(function | ||
| Ok result -> Left result | ||
| Error (`Diagnostic_message message) -> Right message) | ||
in | ||
match results with | ||
| [] -> Error errors | ||
| x :: xs -> | ||
Ok (List.fold_left xs ~init:x ~f:Dune_pkg.Opam_solver.Solver_result.merge, errors) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From an API perspective I find it somewhat weird that I can have an Maybe better something like |
||
;; | ||
|
||
let solve_single_env | ||
solver_env | ||
version_preference | ||
repos | ||
~pins | ||
~local_packages | ||
~constraints | ||
= | ||
let open Fiber.O in | ||
let+ result = | ||
Dune_pkg.Opam_solver.solve_lock_dir | ||
solver_env | ||
version_preference | ||
repos | ||
~pins | ||
~local_packages | ||
~constraints | ||
in | ||
match result with | ||
| Ok result -> Ok (result, []) | ||
| Error (`Diagnostic_message message) -> Error [ message ] | ||
;; | ||
|
||
let solve_lock_dir | ||
workspace | ||
~local_packages | ||
~project_pins | ||
~print_perf_stats | ||
~portable_lock_dir | ||
version_preference | ||
solver_env_from_current_system | ||
lock_dir_path | ||
|
@@ -109,7 +174,8 @@ let solve_lock_dir | |
let* pins = resolve_project_pins project_pins in | ||
let time_solve_start = Unix.gettimeofday () in | ||
progress_state := Some Progress_indicator.Per_lockdir.State.Solving; | ||
Dune_pkg.Opam_solver.solve_lock_dir | ||
let solve = if portable_lock_dir then solve_multiple_envs else solve_single_env in | ||
solve | ||
solver_env | ||
(Pkg_common.Version_preference.choose | ||
~from_arg:version_preference | ||
|
@@ -121,8 +187,11 @@ let solve_lock_dir | |
(Package_name.Map.map local_packages ~f:Dune_pkg.Local_package.for_solver) | ||
~constraints:(constraints_of_workspace workspace ~lock_dir_path) | ||
>>= function | ||
| Error (`Diagnostic_message message) -> Fiber.return (Error (lock_dir_path, message)) | ||
| Ok { lock_dir; files; pinned_packages; num_expanded_packages } -> | ||
| Error messages -> Fiber.return (Error (lock_dir_path, messages)) | ||
| Ok ({ lock_dir; files; pinned_packages; num_expanded_packages }, _errors) -> | ||
(* TODO: Users might want to know if no solution was found on certain | ||
platforms. Give the option to print the solver errors, even if a | ||
solution was found on some platforms. *) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think if we were to print at least a warning like I don't have a good solution to how to deal with debugging them. On one hand outputting the solver error for every failed configuration is extremely verbose, on the other hand just displaying "couldn't solve" does not help people to find a solution. Maybe a flag to display the verbose lock errors? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should log it and tell the user to inspect the failure in |
||
let time_end = Unix.gettimeofday () in | ||
let maybe_perf_stats = | ||
if print_perf_stats | ||
|
@@ -149,7 +218,13 @@ let solve_lock_dir | |
in | ||
progress_state := None; | ||
let+ lock_dir = Lock_dir.compute_missing_checksums ~pinned_packages lock_dir in | ||
Ok (Lock_dir.Write_disk.prepare ~lock_dir_path ~files lock_dir, summary_message) | ||
Ok | ||
( Lock_dir.Write_disk.prepare | ||
~portable:portable_lock_dir | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe make the argument |
||
~lock_dir_path | ||
~files | ||
lock_dir | ||
, summary_message ) | ||
;; | ||
|
||
let solve | ||
|
@@ -160,6 +235,7 @@ let solve | |
~version_preference | ||
~lock_dirs | ||
~print_perf_stats | ||
~portable_lock_dir | ||
= | ||
let open Fiber.O in | ||
(* a list of thunks that will perform all the file IO side | ||
|
@@ -182,6 +258,7 @@ let solve | |
~local_packages | ||
~project_pins | ||
~print_perf_stats | ||
~portable_lock_dir | ||
version_preference | ||
solver_env_from_current_system | ||
lockdir_path | ||
|
@@ -196,9 +273,9 @@ let solve | |
| Error errors -> | ||
User_error.raise | ||
([ Pp.text "Unable to solve dependencies for the following lock directories:" ] | ||
@ List.concat_map errors ~f:(fun (path, message) -> | ||
@ List.concat_map errors ~f:(fun (path, messages) -> | ||
[ Pp.textf "Lock directory %s:" (Path.Source.to_string_maybe_quoted path) | ||
; Pp.hovbox message | ||
; Pp.hovbox (Pp.concat ~sep:Pp.newline messages) | ||
])) | ||
| Ok write_disks_with_summaries -> | ||
let write_disk_list, summary_messages = List.split write_disks_with_summaries in | ||
|
@@ -214,7 +291,7 @@ let project_pins = | |
Pin_stanza.DB.combine_exn acc (Dune_project.pins project)) | ||
;; | ||
|
||
let lock ~version_preference ~lock_dirs_arg ~print_perf_stats = | ||
let lock ~version_preference ~lock_dirs_arg ~print_perf_stats ~portable_lock_dir = | ||
let open Fiber.O in | ||
let* solver_env_from_current_system = | ||
Dune_pkg.Sys_poll.make ~path:(Env_path.path Stdune.Env.initial) | ||
|
@@ -240,6 +317,7 @@ let lock ~version_preference ~lock_dirs_arg ~print_perf_stats = | |
~version_preference | ||
~lock_dirs | ||
~print_perf_stats | ||
~portable_lock_dir | ||
;; | ||
|
||
let term = | ||
|
@@ -250,7 +328,12 @@ let term = | |
let builder = Common.Builder.forbid_builds builder in | ||
let common, config = Common.init builder in | ||
Scheduler.go ~common ~config (fun () -> | ||
lock ~version_preference ~lock_dirs_arg ~print_perf_stats) | ||
let portable_lock_dir = | ||
match Config.get Dune_rules.Compile_time.portable_lock_dir with | ||
| `Enabled -> true | ||
| `Disabled -> false | ||
in | ||
lock ~version_preference ~lock_dirs_arg ~print_perf_stats ~portable_lock_dir) | ||
;; | ||
|
||
let info = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ let default_toggles : (string * [ `Disabled | `Enabled ]) list = | |
; "pkg_build_progress", `Disabled | ||
; "lock_dev_tool", `Disabled | ||
; "bin_dev_tools", `Disabled | ||
; "portable_lock_dir", `Disabled | ||
] | ||
;; | ||
|
||
|
@@ -110,6 +111,10 @@ let () = | |
, " Enable obtaining dev-tools binarys from the binary package opam repository. \ | ||
Allows fast installation of dev-tools. \n\ | ||
\ This flag is experimental and shouldn't be relied on by packagers." ) | ||
; ( "--portable-lock-dir" | ||
, toggle "portable_lock_dir" | ||
, "Generate portable lock dirs. If this feature is disabled then lock dirs will be \ | ||
specialized to the machine where they are generated." ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to enable this for the developer preview by default? Probably yes? |
||
] | ||
in | ||
let anon s = bad "Don't know what to do with %s" s in | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,6 @@ type t = | |
{ original : source | ||
; local_file : Path.Local.t | ||
} | ||
|
||
val equal : t -> t -> bool | ||
val to_dyn : t -> Dyn.t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that
TODO
done? By the code it seems like.