-
Notifications
You must be signed in to change notification settings - Fork 377
Clean all created temporary files at exit #6695
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: master
Are you sure you want to change the base?
Conversation
| val with_tmp_dir_job: (Dir.t -> 'a OpamProcess.job) -> 'a OpamProcess.job | ||
|
|
||
| (** Raw function to create a temporary directory. No automatic cleanup *) | ||
| (** Raw function to create a temporary directory. |
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.
These mk_* functions are misleading, they do not create anything but merely return a unique name. I think we should change this first
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.
good point. Especially with https://github.com/ocaml/opam/pull/6659/files#diff-38a34666fa25190dcc42466d8decbd8c6d5bb62952d05b09f0d233c37b3bd4d7R320 we have a case where we don't want to clean the temporary file, we should have both functions.
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.
As discussed, there is indeed no need to have the non cleaned temporary files for opamfile variable as we stay in opam.
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.
These mk_* functions are misleading, they do not create anything but merely return a unique name. I think we should change this first
I've opened #6704 for that
src/core/opamFilename.ml
Outdated
| if exists filename then Some filename else None | ||
|
|
||
| let mk_tmp_file () = | ||
| of_string @@ OpamSystem.mk_temp_dir () |
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.
creating new files by ""creating a directory"" feels a bit wrong
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.
It is because it is non well named, as you pointed on your previous message.
| if Sys.file_exists d then | ||
| Unix.rmdir d; |
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.
The check is redundant with the try ... with Unix_error _ block
| if Sys.file_exists d then | |
| Unix.rmdir d; | |
| Unix.rmdir d; |
No description provided.