Skip to content

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.

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

Open
rjbou wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Clean all created temporary files at exit #6695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b381ec8
reftest: update OPAMTMP regexp
rjbou Sep 11, 2025
7406472
system: force cleaning all temporary directory created
rjbou Sep 11, 2025
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
3 changes: 2 additions & 1 deletion src/core/opamFilename.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ val with_tmp_dir: (Dir.t -> 'a) -> 'a
(** Provide an automatically cleaned up temp directory to a job *)
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.
Copy link
Member

@kit-ty-kate kit-ty-kate Sep 17, 2025

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

Copy link
Collaborator Author

@rjbou rjbou Sep 18, 2025

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.

Copy link
Collaborator Author

@rjbou rjbou Sep 23, 2025

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.

Copy link
Collaborator Author

@rjbou rjbou Sep 23, 2025 •
edited
Loading

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

If it exists, cleaned automatically at exit. *)
val mk_tmp_dir: unit -> Dir.t

(** Create a new Dir.t and resolve symlinks *)
Expand Down
19 changes: 18 additions & 1 deletion src/core/opamSystem.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,29 @@ let temp_name ?dir ?(prefix="opam") () =
in
tmpdir / (temp_basename prefix)

let temp_dir_cleaner =
let to_clean = ref OpamStd.String.Set.empty in
OpamStd.Sys.at_exit
(fun () ->
OpamStd.String.Set.iter (fun d ->
try
if Sys.file_exists d then
Unix.rmdir d;
Comment on lines +98 to +99
Copy link
Member

@kit-ty-kate kit-ty-kate Sep 23, 2025

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

Suggested change
if Sys.file_exists d then
Unix.rmdir d;
Unix.rmdir d;

(* Only log the item if unlink succeeded *)
log ~level:4 "temp_dir_cleaner: rm: %s" d
with Unix.Unix_error _ -> ())
!to_clean);
fun tmp_dir ->
to_clean := OpamStd.String.Set.add tmp_dir !to_clean

let rec mk_temp_dir ?prefix () =
let s = temp_name ?prefix () in
if Sys.file_exists s then
mk_temp_dir ?prefix ()
else
real_path s
let dir = real_path s in
temp_dir_cleaner dir;
dir

let rec mk_unique_dir ~dir ?(prefix="opam") () =
let s = dir / Printf.sprintf "%s-%06x" prefix (Random.int 0xFFFFFF) in
Expand Down
5 changes: 3 additions & 2 deletions src/core/opamSystem.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ val verbose_for_base_commands: unit -> bool

(** {2 Filesystem management} *)

(** Returns a directory name, in the temporary directory, composed by {i opam}
(if [prefix] is not set), pid, and random number. *)
(** As [mk_temp] command, returns a directory name, in the temporary directory,
composed by {i opam} (if [prefix] is not set), pid, and random number.
Cleaned at exit. *)
val mk_temp_dir: ?prefix:string -> unit -> string

(** Returns a directory name, in the [~dir], composed by {i opam}
Expand Down
47 changes: 47 additions & 0 deletions tests/reftests/action-disk.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ SYSTEM LOCK ${BASEDIR}/OPAM/install-from-repo/.opam-swi
SYSTEM rm ${BASEDIR}/OPAM/install-from-repo/.opam-switch/backup/state-today.export
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### opam reinstall main-repo.1 | sed-cmd tar cp touch mkdir | "${PRE_MD5}..?${MD5}" -> pre/hash | unordered
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
Expand Down Expand Up @@ -290,6 +293,7 @@ SYSTEM LOCK ${BASEDIR}/OPAM/install-from-repo/.opam-swi
SYSTEM rm ${BASEDIR}/OPAM/install-from-repo/.opam-switch/backup/state-today.export
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### opam upgrade main-repo | sed-cmd tar touch mkdir cp | "${PRE_MD5}..?${MD5}" -> pre/hash | unordered
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
Expand Down Expand Up @@ -341,6 +345,8 @@ Processing 2/4: [main-repo: touch build-file]
-> compiled main-repo.2
SYSTEM rmdir ${BASEDIR}/OPAM/install-from-repo/.opam-switch/remove/main-repo.1
SYSTEM copydir ${BASEDIR}/OPAM/install-from-repo/.opam-switch/sources/main-repo.1 -> ${BASEDIR}/OPAM/install-from-repo/.opam-switch/remove/main-repo.1
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM mkdir ${BASEDIR}/OPAM/install-from-repo/.opam-switch/remove/main-repo.1
SYSTEM copy ${BASEDIR}/OPAM/install-from-repo/.opam-switch/sources/main-repo.1/content -> ${BASEDIR}/OPAM/install-from-repo/.opam-switch/remove/main-repo.1/content
Processing 3/4: [main-repo: touch removal-file]
Expand Down Expand Up @@ -457,6 +463,7 @@ SYSTEM LOCK ${BASEDIR}/OPAM/install-from-repo/.opam-swi
SYSTEM rm ${BASEDIR}/OPAM/install-from-repo/.opam-switch/backup/state-today.export
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### :II: Pin
### :II:1: path pin
### <pin:main-ppin/main-ppin.opam>
Expand Down Expand Up @@ -554,7 +561,12 @@ SYSTEM LOCK ${BASEDIR}/OPAM/install-from-path-pin-all/.
SYSTEM rm ${BASEDIR}/OPAM/install-from-path-pin-all/.opam-switch/backup/state-today.export
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### opam install main-ppin | grep -v '^- ./$' | sed-cmd rsync cp touch mkdir | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -633,6 +645,9 @@ SYSTEM rm ${BASEDIR}/OPAM/install-from-path-pin-all/.op
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
### opam reinstall main-ppin | grep -v '^- ./$' | sed-cmd rsync cp touch mkdir | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -731,6 +746,8 @@ SYSTEM rm ${BASEDIR}/OPAM/install-from-path-pin-all/.op
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
### opam remove main-ppin | grep -v '^- ./$' | sed-cmd rsync touch | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -914,7 +931,12 @@ SYSTEM LOCK ${BASEDIR}/OPAM/install-from-path-pin-all/.
SYSTEM rm ${BASEDIR}/OPAM/install-from-path-pin-all/.opam-switch/backup/state-today.export
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### opam reinstall ./main-ppin | grep -v '^- ./$' | sed-cmd rsync cp touch mkdir | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -1012,6 +1034,8 @@ SYSTEM rm ${BASEDIR}/OPAM/install-from-path-pin-all/.op
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
### opam unpin main-ppin -y | grep -v '^- ./$' | sed-cmd rsync touch | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -1199,7 +1223,12 @@ SYSTEM LOCK ${BASEDIR}/OPAM/install-from-git-pin-all/.o
SYSTEM rm ${BASEDIR}/OPAM/install-from-git-pin-all/.opam-switch/backup/state-today.export
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### opam install main-gpin | sed-cmd git cp touch mkdir | grep -v "(mkdir|copy|copydir|rm) .*\\.git." | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -1282,6 +1311,9 @@ SYSTEM rm ${BASEDIR}/OPAM/install-from-git-pin-all/.opa
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
### opam reinstall main-gpin | sed-cmd git cp touch mkdir | grep -v "(mkdir|copy|copydir|rm) .*\\.git." | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -1391,6 +1423,8 @@ SYSTEM rm ${BASEDIR}/OPAM/install-from-git-pin-all/.opa
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
### opam remove main-gpin | sed-cmd git touch | grep -v "(mkdir|copy|copydir|rm) .*\\.git." | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -1587,7 +1621,12 @@ SYSTEM LOCK ${BASEDIR}/OPAM/install-from-git-pin-all/.o
SYSTEM rm ${BASEDIR}/OPAM/install-from-git-pin-all/.opam-switch/backup/state-today.export
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### opam reinstall ./main-gpin | sed-cmd git cp touch mkdir | grep -v "(mkdir|copy|copydir|rm) .*\\.git." | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -1696,6 +1735,8 @@ SYSTEM rm ${BASEDIR}/OPAM/install-from-git-pin-all/.opa
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
### opam unpin main-gpin -y | sed-cmd git touch | grep -v "(mkdir|copy|copydir|rm) .*\\.git." | grep -v "HEAD is now" | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -1917,7 +1958,9 @@ SYSTEM LOCK ${BASEDIR}/OPAM/install-from-version-pin/.o
SYSTEM rm ${BASEDIR}/OPAM/install-from-version-pin/.opam-switch/backup/state-today.export
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### opam reinstall main-repo | grep -v '^- ./$' | sed-cmd tar rsync cp touch mkdir | "${PRE_MD5}..?${MD5}" -> pre/hash | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -2003,6 +2046,7 @@ SYSTEM rm ${BASEDIR}/OPAM/install-from-version-pin/.opa
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
### opam remove main-repo | grep -v '^- ./$' | sed-cmd tar rsync touch | "${PRE_MD5}..?${MD5}" -> pre/hash | unordered
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
SYSTEM LOCK ${BASEDIR}/OPAM/lock (none => read)
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -2193,6 +2237,7 @@ SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (write => none)
SYSTEM LOCK ${BASEDIR}/OPAM/package-switch/.opam-switch/lock (write => none)
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### :III:3: from local, empty
### opam switch create ./void | unordered
FILE(config) Read ${BASEDIR}/OPAM/config in 0.000s
Expand Down Expand Up @@ -2350,6 +2395,8 @@ SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (write => none)
SYSTEM LOCK ${BASEDIR}/main-ppin/_opam/.opam-switch/lock (write => none)
SYSTEM LOCK ${BASEDIR}/OPAM/repo/lock (none => none)
SYSTEM LOCK ${BASEDIR}/OPAM/config.lock (none => none)
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
SYSTEM temp_dir_cleaner: rm: ${OPAMTMP}
### :IV: Switch deletion
### :IV:1: local
### opam switch remove ./main-ppin | unordered
Expand Down
1 change: 1 addition & 0 deletions tests/reftests/run.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ let common_filters ?opam dir =
alt (dir_to_regex tmpdir);
rep (set "/\\");
str "opam-";
opt @@ str "pin-cache-";
rep1 (alt [xdigit; char '-'])],
Sed "${OPAMTMP}";
seq [
Expand Down
Loading