Skip to content
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.

Sign up for GitHub

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

Fix environment file handling for empty switches #3899

Merged
merged 3 commits into from
Jul 3, 2019
Merged

Fix environment file handling for empty switches #3899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d4e4ae
Always write environment file on creation
dra27 Jun 28, 2019
c93188e
Display env warning for empty switches
dra27 Jun 28, 2019
b3b8716
opam config revert-env regenerates environment
dra27 Jun 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/client/opamCommands.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,9 +919,14 @@ let config =
~set_opamroot ~set_opamswitch
~csh:(shell=SH_csh) ~sexp ~fish:(shell=SH_fish) ~inplace_path))
| Some `revert_env, [] ->
`Ok (OpamConfigCommand.print_eval_env
~csh:(shell=SH_csh) ~sexp ~fish:(shell=SH_fish)
(OpamEnv.add [] []))
OpamGlobalState.with_ `Lock_none @@ fun gt ->
(match OpamStateConfig.get_switch_opt () with
| None -> `Ok ()
| Some sw ->
`Ok (OpamConfigCommand.ensure_env gt sw;
OpamConfigCommand.print_eval_env
~csh:(shell=SH_csh) ~sexp ~fish:(shell=SH_fish)
(OpamEnv.add [] [])))
| Some `exec, (_::_ as c) ->
OpamGlobalState.with_ `Lock_none @@ fun gt ->
`Ok (OpamConfigCommand.exec
Expand Down
42 changes: 25 additions & 17 deletions src/client/opamConfigCommand.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,25 @@ let print_eval_env ~csh ~sexp ~fish env =
else
print_env env

let ensure_env_aux ?(set_opamroot=false) ?(set_opamswitch=false) ?(force_path=true) gt switch =
let env_file = OpamPath.Switch.environment gt.root switch in
if not (OpamFile.exists env_file) then
Some (OpamSwitchState.with_ `Lock_none gt @@ fun st ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good reason to not take the write lock directly in this case ? Lock upgrades should be avoided unless absolutely necessary (and the scenarios for deadlocks have been considered)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to change this, but this is a refactoring - it's exactly the code deleted below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right! I should ask @rjbou then :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

env file is only regenerated & written if a write lock can be taken (not --safe, cf. 3691). We don't want to block opam env because of the missing environment file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, was I was suggesting is stg along the lines of:

  if not safe_mode && not OpamFile.exists env_file then OpamSwitchState.with_ `Lock_write gt @@ ... (* regenerate *)
  else OpamSwitchState.with_ `Lock_none gt @@ ...

the point would just be to avoid the use of OpamSwitchState.with_write_lock really, since we could detect the need beforehand and load with a write lock directly, which is always better & safer than upgrading existing locks.

let upd =
OpamEnv.updates ~set_opamroot ~set_opamswitch ~force_path st
in
log "Missing environment file, regenerate it";
if not (OpamCoreConfig.(!r.safe_mode)) then
(let _, st =
OpamSwitchState.with_write_lock st @@ fun st ->
(OpamFile.Environment.write env_file upd), st
in OpamSwitchState.drop st);
OpamEnv.add [] upd)
else
None

let ensure_env gt switch = ignore (ensure_env_aux gt switch)

let env gt switch ?(set_opamroot=false) ?(set_opamswitch=false)
~csh ~sexp ~fish ~inplace_path =
log "config-env";
Expand Down Expand Up @@ -256,23 +275,12 @@ let env gt switch ?(set_opamroot=false) ?(set_opamswitch=false)
(OpamConsole.colorise `bold "OPAMSWITCH");
let force_path = not inplace_path in
let env =
let env_file = OpamPath.Switch.environment gt.root switch in
if not (OpamFile.exists env_file) then
(OpamSwitchState.with_ `Lock_none gt @@ fun st ->
let upd =
OpamEnv.updates ~set_opamroot ~set_opamswitch ~force_path st
in
log "Missing environment file, regenerates it";
if not (OpamCoreConfig.(!r.safe_mode)) then
(let _, st =
OpamSwitchState.with_write_lock st @@ fun st ->
(OpamFile.Environment.write env_file upd), st
in OpamSwitchState.drop st);
OpamEnv.add [] upd)
else
OpamEnv.get_opam_raw
~set_opamroot ~set_opamswitch ~force_path
gt.root switch
match ensure_env_aux ~set_opamroot ~set_opamswitch ~force_path gt switch with
| Some env -> env
| None ->
OpamEnv.get_opam_raw
~set_opamroot ~set_opamswitch ~force_path
gt.root switch
in
print_eval_env ~csh ~sexp ~fish env

Expand Down
4 changes: 4 additions & 0 deletions src/client/opamConfigCommand.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ val env:
csh:bool -> sexp:bool -> fish:bool -> inplace_path:bool ->
unit

(** Ensures that the environment file exists in the given switch, regenerating
it, if necessary. *)
val ensure_env: 'a global_state -> switch -> unit

(** Like [env] but allows one to specify the precise env to print rather than
compute it from a switch state *)
val print_eval_env: csh:bool -> sexp:bool -> fish:bool -> env -> unit
Expand Down
6 changes: 5 additions & 1 deletion src/client/opamSwitchCommand.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,11 @@ let remove gt ?(confirm = true) switch =

let install_compiler_packages t atoms =
(* install the compiler packages *)
if atoms = [] then t else
if atoms = [] then begin
OpamFile.Environment.write (OpamPath.Switch.environment t.switch_global.root t.switch) (OpamEnv.compute_updates t);
OpamEnv.check_and_print_env_warning t;
t
AltGr marked this conversation as resolved.
Show resolved Hide resolved
end else
let roots = OpamPackage.Name.Set.of_list (List.map fst atoms) in
let not_found =
OpamPackage.Name.Set.diff roots @@
Expand Down