Skip to content

Remove the build directory as soon as possible when installing a package #6906

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
kit-ty-kate wants to merge 2 commits into
base: master
Choose a base branch
from
+56 −7
Open

Remove the build directory as soon as possible when installing a package #6906

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
3653413
Add a benchmark for opam install
kit-ty-kate Apr 28, 2026
db02709
Remove the build directory as soon as possible when installing a package
kit-ty-kate Apr 16, 2026
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
2 changes: 2 additions & 0 deletions master_changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ users)
## Actions

## Install
* Remove the build directory as soon as possible when installing a package [#6906 @kit-ty-kate - fix #5884]

## Build (package)

Expand Down Expand Up @@ -148,6 +149,7 @@ users)

## Benchmarks
* Add an even larger real-world diff to benchmark `opam update` [#6567 @kit-ty-kate]
* Add a benchmark for `opam install` [#6912 @kit-ty-kate]

## Reftests
### Tests
Expand Down
18 changes: 11 additions & 7 deletions src/client/opamSolution.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,14 @@ let parallel_apply t
| Left conf ->
add_to_install nv conf;
store_time ();
if not OpamClientConfig.(!r.keep_build_dir) then begin
(* NOTE: build_dir as defined above is the
"build dir in case of --inplace" so != OpamPath.Switch.build *)
let build_dir =
OpamPath.Switch.build t.switch_global.root t.switch nv
in
OpamFilename.rmdir build_dir;
end;
Done (`Successful (OpamPackage.Set.add nv installed, removed))
| Right exn ->
store_time ();
Expand Down Expand Up @@ -864,17 +872,13 @@ let parallel_apply t
end;

let cleanup_artefacts graph =
(* NOTE: build dir removal is done directly after install
instead of here, for performance reasons. *)
PackageActionGraph.iter_vertex (function
| `Remove nv ->
OpamAction.cleanup_package_artefacts t nv
(* if reinstalled, only removes build dir *)
Copy link
Copy Markdown
Collaborator

@rjbou rjbou Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is tied to which line ?

Copy link
Copy Markdown
Member Author

@kit-ty-kate kit-ty-kate Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the one above. See b6ed206

| `Install nv ->
if not OpamClientConfig.(!r.keep_build_dir) then
let build_dir =
OpamPath.Switch.build t.switch_global.root t.switch nv
in
OpamFilename.rmdir build_dir
| `Build _ | `Fetch _ -> ()
| `Build _ | `Fetch _ | `Install _ -> ()
| `Change _ | `Reinstall _ -> assert false)
graph
in
Expand Down
43 changes: 43 additions & 0 deletions tests/bench/bench.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,37 @@ let () =
in
List.fold_left (+.) 0.0 l /. float_of_int n
in
let time_install_core =
Gc.compact ();
launch (fmt "%s switch create -y eight ocaml-base-compiler.4.14.0" bin);
launch (fmt "%s install --download-only -y core.v0.15.1" bin);
launch "test -d ~/.opam/eight/.opam-switch/sources && rm -rf ~/.opam/eight/.opam-switch/sources";
launch "test -d ~/.opam/eight/.opam-switch/build && rm -rf ~/.opam/eight/.opam-switch/build";
time_cmd ~exit:0 (fmt "%s install -y core.v0.15.1" bin)
in
let time_install_big =
Gc.compact ();
launch (fmt "%s switch create nine --empty" bin);
OpamSystem.write "/tmp/opam-bench-bigpkg/repo" {|opam-version: "2.0"
|};
OpamSystem.write "/tmp/opam-bench-bigpkg/packages/a.1/opam" {|opam-version: "2.0"
url {
src: "https://github.com/ocaml/opam-repository/archive/refs/tags/2025-01-before-archiving-phase1.tar.gz"
checksum: "md5=215a5261ad8358e34eb3222b83c4c8be"
}
|};
OpamSystem.write "/tmp/opam-bench-bigpkg/packages/b.1/opam" {|opam-version: "2.0"
build: ["rm" "-r" "packages"]
url {
src: "https://github.com/ocaml/opam-repository/archive/refs/tags/2025-01-before-archiving-phase1.tar.gz"
checksum: "md5=215a5261ad8358e34eb3222b83c4c8be"
}
|};
launch (fmt "%s repo add tmp /tmp/opam-bench-bigpkg" bin);
let res = time_cmd ~exit:0 (fmt "%s install a.1 b.1" bin) in
launch (fmt "%s repo remove tmp -a" bin);
res
in
let init_root tmp_root_dir repo =
launch (fmt "rm -rf %s" tmp_root_dir);
launch (fmt "mkdir -p %s" tmp_root_dir);
Expand Down Expand Up @@ -279,6 +310,16 @@ let () =
"name": "OpamStd.String.split amortised over 10 runs",
"value": %f,
"units": "secs"
},
{
"name": "opam install core.v0.15.1",
"value": %f,
"units": "secs"
},
{
"name": "opam install large-pkg large-pkg2",
"value": %f,
"units": "secs"
}
]
},
Expand Down Expand Up @@ -363,6 +404,8 @@ let () =
time_show_raw
time_show_precise
time_OpamStd_String_split_10
time_install_core
time_install_big
time_update_no_diff_local
time_update_no_diff_git
time_update_small_diff_local
Expand Down
Loading