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.

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

Merged
merged 5 commits into from
Mar 22, 2023

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Mar 10, 2023

Extracted from #5417, which now just deals with the fully revertible environment updates part.

  • Writing the environment file in opam env is still taking the switch write lock, which isn't necessary
    • we keep taking write lock as something is written in switch
  • Add hook specific read-only option: it is like read-only, but permit environment files writing.
    • We want to keep hooks as readonly. The only downside is that when a local switch is moved, the hook takes some times (switch loading) to recompute environment ; prompt will take that time until an opam commands in the terminal with write lock writes the new environment
      • We can still add the hook-read-only (read-only except for env) in the future, when it makes sense to move a switch
  • Test for moving a switch (verify PATH updates)

@rjbou rjbou mentioned this pull request Mar 10, 2023
1 task
@dra27 dra27 force-pushed the atomic-environment branch 3 times, most recently from 6a04218 to f727898 Compare March 11, 2023 11:10
Copy link
Collaborator

@rjbou rjbou left a 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.

@rjbou rjbou added this to the 2.2.0~alpha milestone Mar 13, 2023
@dra27
Copy link
Member Author

dra27 commented Mar 14, 2023

Test added - up to you whether we want to deal with the lock before or after merge, @rjbou?

@rjbou
Copy link
Collaborator

rjbou commented Mar 14, 2023

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)

@@ -76,6 +76,7 @@ end

module type IO_Arg = sig
val internal : string
val atomic : bool
Copy link
Member

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?

Copy link
Member

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.

Copy link
Collaborator

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

Copy link
Member

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

@rjbou rjbou force-pushed the atomic-environment branch 2 times, most recently from 8cc2d27 to 4d0cc94 Compare March 22, 2023 17:21
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`
@kit-ty-kate
Copy link
Member

Thanks

@dra27 dra27 mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants