Skip to content

Commit

Permalink
Detect rule target & source dir overlap
Browse files Browse the repository at this point in the history
When there's a rule that tries to create a target that is already a
soure directory, this should be reported as an error.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
  • Loading branch information
rgrinberg authored and stephanieyou committed Apr 7, 2020
1 parent e190767 commit 2301907
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 22 deletions.
54 changes: 39 additions & 15 deletions src/dune/build_system.ml
Original file line number Diff line number Diff line change
Expand Up @@ -409,20 +409,38 @@ let get_dir_triage t ~dir =
| Build (Regular (With_context _)) ->
Need_step2

let report_rule_conflict fn (rule' : Rule.t) (rule : Rule.t) =
let describe (rule : Rule.t) =
let describe_rule (rule : Rule.t) =
match rule.info with
| From_dune_file { start; _ } ->
start.pos_fname ^ ":" ^ string_of_int start.pos_lnum
| Internal -> "<internal location>"
| Source_file_copy -> "file present in source tree"

let report_rule_src_dir_conflict dir fn (rule : Rule.t) =
let loc =
match rule.info with
| From_dune_file { start; _ } ->
start.pos_fname ^ ":" ^ string_of_int start.pos_lnum
| Internal -> "<internal location>"
| Source_file_copy -> "file present in source tree"
| From_dune_file loc -> loc
| Internal
| Source_file_copy ->
let dir =
match Path.Build.drop_build_context dir with
| None -> Path.build dir
| Some s -> Path.source s
in
Loc.in_dir dir
in
User_error.raise ~loc
[ Pp.textf "Rule has a target %s" (Path.Build.to_string_maybe_quoted fn)
; Pp.textf "This conflicts with a source directory in the same directory"
]

let report_rule_conflict fn (rule' : Rule.t) (rule : Rule.t) =
let fn = Path.build fn in
User_error.raise
[ Pp.textf "Multiple rules generated for %s:"
(Path.to_string_maybe_quoted fn)
; Pp.textf "- %s" (describe rule')
; Pp.textf "- %s" (describe rule)
; Pp.textf "- %s" (describe_rule rule')
; Pp.textf "- %s" (describe_rule rule)
]
~hints:
( match (rule.info, rule'.info) with
Expand Down Expand Up @@ -597,11 +615,15 @@ end = struct
~sandbox:Sandbox_config.no_sandboxing build ~context:None ~env:None
~info:Source_file_copy)

let compile_rules ~dir rules =
let compile_rules ~dir ~source_dirs rules =
List.concat_map rules ~f:(fun rule ->
assert (Path.Build.( = ) dir rule.Rule.dir);
List.map (Path.Build.Set.to_list rule.action.targets) ~f:(fun target ->
(target, rule)))
Path.Build.Set.to_list rule.action.targets
|> List.map ~f:(fun target ->
if String.Set.mem source_dirs (Path.Build.basename target) then
report_rule_src_dir_conflict dir target rule
else
(target, rule)))
|> Path.Build.Map.of_list_reducei ~f:report_rule_conflict

(* Here we are doing a O(log |S|) lookup in a set S of files in the build
Expand Down Expand Up @@ -709,7 +731,9 @@ end = struct
:: rules)
in
fun ~subdirs_to_keep ->
let rules_here = compile_rules ~dir:alias_dir alias_rules in
let rules_here =
compile_rules ~dir:alias_dir ~source_dirs:String.Set.empty alias_rules
in
remove_old_artifacts ~rules_here ~dir:alias_dir ~subdirs_to_keep;
rules_here

Expand Down Expand Up @@ -915,7 +939,7 @@ end = struct
|> Path.Source.Set.of_list_map ~f:Path.Build.drop_build_context_exn
in
(* Take into account the source files *)
let to_copy, subdirs_to_keep =
let to_copy, source_dirs =
match context_name with
| Install _ -> (None, String.Set.empty)
| Context context_name ->
Expand All @@ -937,7 +961,7 @@ end = struct
let subdirs_to_keep =
match extra_subdirs_to_keep with
| All -> Subdir_set.All
| These set -> These (String.Set.union subdirs_to_keep set)
| These set -> These (String.Set.union source_dirs set)
in
(* Filter out fallback rules *)
let rules =
Expand All @@ -956,7 +980,7 @@ end = struct
create_copy_rules ~ctx_dir ~non_target_source_files:source_files )
@ rules
in
let rules_here = compile_rules ~dir rules in
let rules_here = compile_rules ~dir ~source_dirs rules in
let allowed_by_parent =
Generated_directory_restrictions.allowed_by_parent ~dir
in
Expand Down
13 changes: 6 additions & 7 deletions test/blackbox-tests/test-cases/dir-target-dep/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,12 @@
We should not be able to produce a directory in a rule that already exists
$ dune build --display=short --root no-overlapping-rules
Entering directory 'no-overlapping-rules'
ocamldep .foo.eobjs/foo.ml.d
ocamlc .foo.eobjs/byte/foo.{cmi,cmo,cmt}
ocamlopt .foo.eobjs/native/foo.{cmx,o}
ocamlopt foo.exe
foo dir (exit 2)
(cd _build/default && ./foo.exe dir)
Fatal error: exception Unix.Unix_error(Unix.EEXIST, "mkdir", "dir")
File "dune", line 5, characters 0-51:
5 | (rule
6 | (targets dir)
7 | (action (run ./foo.exe dir)))
Error: Rule has a target default/dir
This conflicts with a source directory in the same directory
[1]

Dune crashes if there's a file named after the directory target
Expand Down

0 comments on commit 2301907

Please sign in to comment.