Fix environment file handling for empty switches #3899
Conversation
opam switch create --empty didn't write the switch environment file, since this was only done if there were packages to install.
opam switch create --empty wouldn't display the out-of-date environment warning, since no packages had been installed.
opam env will regenerate a missing environment file. This is extended to opam config revert-env which would silently fail to do anything if the environment file wasn't present.
| 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 -> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Happy to change this, but this is a refactoring - it's exactly the code deleted below.
There was a problem hiding this comment.
Oh right! I should ask @rjbou then :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
LGTM besides the comment above! |
opam switch create foo --emptydoesn't write anenvironmentfile. At this point,opam config revert-envwill not return any updates.opam envwill regenerate file. This PR fixes bothopam switch create --emptyto write theenvironmentfile and alsoopam config revert-envto regenerate it if it's missing (so belt-and-braces approach). Finally, the environment is also not checked inopam switch create --emptybecause of the same branch, and this is fixed (it hurts Windows, which actually updates the environment at this point; it doesn't hurt Unix if you have the shell hook installed).