-
Notifications
You must be signed in to change notification settings - Fork 359
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
Support atomic environment updates in OpamFile #5476
Conversation
6a04218
to
f727898
Compare
f727898
to
cc1489f
Compare
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.
lgtm!
I updated api changes.
cc1489f
to
2280337
Compare
Test added - up to you whether we want to deal with the lock before or after merge, @rjbou? |
From dev meeting: add hook specific option to permit read-only except for environment file ; it resolves switch lock issue in the same time as there is no clash possible on environment files name (temporary files) |
e26e1ec
to
1fedc3f
Compare
@@ -76,6 +76,7 @@ end | |||
|
|||
module type IO_Arg = sig | |||
val internal : string | |||
val atomic : bool |
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 there a downside to always having all file writes as atomic?
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.
When i tried to implemented this feature before i ended up to the same conclusion but only because i didn't think of using Filename.open_temp_file ~temp_dir:dst_dir
which takes care of the only problem i was thinking of, being: x.tmp
already exists.
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.
Atomic takes few more steps: first write on tmp, then rename on final location, instead of directly writing on final location.
related #5473
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.
Yes but that's fine. I don't think i'm able to see any usecase where this isn't better
8cc2d27
to
4d0cc94
Compare
Files can now specify a boolean flag `atomic` to indicate whether the file contents should be updated atomically.
Existing code path was inconsistent w.r.t opam env. We should expect equivalent behaviour from `eval $(opam env)` and `opam exec -- $SHELL`
4d0cc94
to
18741ab
Compare
Thanks |
Extracted from #5417, which now just deals with the fully revertible environment updates part.
opam env
is still taking the switch write lock, which isn't necessaryPATH
updates)