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

[RFC] Allow dune to treat targets opaquely. #1547

Merged
3 commits merged into from
Nov 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ next
- Fix `dune build @doc` deleting `highlight.pack.js` on rebuilds, after the
first build (#1557, @aantron).

- Allow targets to be directories, which Dune will treat opaquely
(#1547, @jordwalke)


1.5.1 (7/11/2018)
-----------------

Expand Down
30 changes: 15 additions & 15 deletions src/build_system.ml
Original file line number Diff line number Diff line change
Expand Up @@ -705,21 +705,21 @@ let remove_old_artifacts t ~dir ~subdirs_to_keep =
| files ->
List.iter files ~f:(fun fn ->
let path = Path.relative dir fn in
match Unix.lstat (Path.to_string path) with
| { st_kind = S_DIR; _ } -> begin
match subdirs_to_keep with
| All -> ()
| These set ->
if String.Set.mem set fn ||
Path.Set.mem t.build_dirs_to_keep path then
()
else
Path.rm_rf path
end
| exception _ ->
if not (Path.Table.mem t.files path) then Path.unlink path
| _ ->
if not (Path.Table.mem t.files path) then Path.unlink path)
let path_is_a_target = Path.Table.mem t.files path in
if path_is_a_target then ()
else
match Unix.lstat (Path.to_string path) with
| { st_kind = S_DIR; _ } -> begin
match subdirs_to_keep with
| All -> ()
| These set ->
if String.Set.mem set fn ||
Path.Set.mem t.build_dirs_to_keep path then ()
else
Path.rm_rf path
end
| exception _ -> Path.unlink path
| _ -> Path.unlink path)

let no_rule_found =
let fail fn ~loc =
Expand Down
24 changes: 19 additions & 5 deletions src/utils.ml
Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,26 @@ module Cached_digest = struct
; table = Hashtbl.create 1024
}

let dir_digest stat =
let ints = stat.Unix.st_size + stat.st_perm in
(string_of_int ints ^
string_of_float stat.st_mtime ^
string_of_float stat.st_ctime)
Copy link
Member

Choose a reason for hiding this comment

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

It's a relatively minor thing, but (a ^ b ^ c) will allocate an intermediate string (a^b) that creates unnecessary garbage. It might be better to do a sprintf here to do it in one allocation if the digest function is called a lot

Copy link

Choose a reason for hiding this comment

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

Now that you point that out, I just realise that we are missing separators in this string. i.e. two different stats records might produce the same string... That's very unlikely, but just to avoid readers wondering about it, we should add separators

Copy link
Member

Choose a reason for hiding this comment

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

Fix here #1592

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you all!


let path_stat_digest fn stat =
if stat.Unix.st_kind = Unix.S_DIR then
dir_digest stat
else Digest.file (Path.to_string fn)

let refresh fn =
let digest = Digest.file (Path.to_string fn) in
let path = Path.to_string fn in
let stat = Unix.stat path in
let digest = path_stat_digest fn stat in
Hashtbl.replace cache.table ~key:fn
~data:{ digest
; timestamp = (Unix.stat (Path.to_string fn)).st_mtime
; timestamp = stat.st_mtime
; timestamp_checked = cache.checked_key
};
};
digest

let file fn =
Expand All @@ -218,9 +231,10 @@ module Cached_digest = struct
if x.timestamp_checked = cache.checked_key then
x.digest
else begin
let mtime = (Unix.stat (Path.to_string fn)).st_mtime in
let stat = Unix.stat (Path.to_string fn) in
let mtime = stat.st_mtime in
if mtime <> x.timestamp then begin
let digest = Digest.file (Path.to_string fn) in
let digest = path_stat_digest fn stat in
x.digest <- digest;
x.timestamp <- mtime;
end;
Expand Down