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.

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

Infer rule targets in long-form rule stanzas as well #2494

Merged
merged 4 commits into from
Aug 2, 2019
Merged

Infer rule targets in long-form rule stanzas as well #2494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
437e2ab
Add basic tests for rule target inferrence
NathanReb Jul 31, 2019
29458b1
Remove existing inferred rule test in misc
NathanReb Aug 1, 2019
690714c
Make target or targets optional fields of rule stanzas
NathanReb Aug 1, 2019
04f60a4
Add test-cases for rule inferrence error
NathanReb Aug 1, 2019
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: 3 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
2.0.0 (unreleased)
------------------

- Infer targets for rule stanzas expressed in long form (#2494, fixes #2469,
@NathanReb)

- Indicate the progress of the initial file tree loading (#2459, fixes #2374,
@bobot)

Expand Down
1 change: 1 addition & 0 deletions src/dune_file.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,7 @@ module Rule = struct

let fields_parser =
fields_mutually_exclusive
~default:Infer
[ "targets", decode_static
; "target", decode_one_static
]
Expand Down
7 changes: 5 additions & 2 deletions src/dune_lang/dune_lang.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,7 +1048,7 @@ module Decoder = struct
[@@inline never]

let fields_mutually_exclusive
?on_dup fields ((Fields (loc, _, _) : _ context) as ctx) state =
?on_dup ?default fields ((Fields (loc, _, _) : _ context) as ctx) state =
let res, state =
traverse ~f:(fun (name, parser) ->
field_o name ?on_dup parser
Expand All @@ -1060,7 +1060,10 @@ module Decoder = struct
~f:(function (name, Some x) -> Some(name, x) | (_, None) -> None) with
| [] ->
let names = List.map fields ~f:fst in
fields_missing_need_exactly_one loc names
( match default with
| None -> fields_missing_need_exactly_one loc names
| Some default -> default, state
)
| [ (_name, res) ] ->
res, state
| (_ :: _ :: _ as results) ->
Expand Down
3 changes: 3 additions & 0 deletions src/dune_lang/dune_lang.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,11 @@ module Decoder : sig
-> 'a t
-> 'a option fields_parser

(** Parser for mutually exclusive fields. If [default] is provided, allow
fields absence. *)
val fields_mutually_exclusive
: ?on_dup:(Univ_map.t -> string -> Ast.t list -> unit)
-> ?default:'a
-> (string * 'a t) list
-> 'a fields_parser

Expand Down
10 changes: 10 additions & 0 deletions test/blackbox-tests/dune.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,6 +1330,14 @@
test-cases/rule-target-external
(progn (run %{exe:cram.exe} -test run.t) (diff? run.t run.t.corrected)))))

(alias
(name rule-target-inferrence)
(deps (package dune) (source_tree test-cases/rule-target-inferrence))
(action
(chdir
test-cases/rule-target-inferrence
(progn (run %{exe:cram.exe} -test run.t) (diff? run.t run.t.corrected)))))

(alias
(name sandboxing)
(deps (package dune) (source_tree test-cases/sandboxing))
Expand Down Expand Up @@ -1833,6 +1841,7 @@
(alias redirections)
(alias reporting-of-cycles)
(alias rule-target-external)
(alias rule-target-inferrence)
(alias sandboxing)
(alias scope-bug)
(alias scope-ppx-bug)
Expand Down Expand Up @@ -2016,6 +2025,7 @@
(alias redirections)
(alias reporting-of-cycles)
(alias rule-target-external)
(alias rule-target-inferrence)
(alias sandboxing)
(alias scope-bug)
(alias select)
Expand Down
9 changes: 0 additions & 9 deletions test/blackbox-tests/test-cases/misc/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,6 @@
(deps result2 expected2)
(action (run diff -u result2 expected2)))

;; Test inferred rules

(rule (copy dune dune-plop))

(alias
(name runtest)
(deps dune dune-plop)
(action (run diff -u %{deps})))

;; For some tests in subdirs

(rule (with-stdout-to 023e1a58-4d08-11e7-a041-aa000008c8a6 (echo "plop")))
Expand Down
1 change: 0 additions & 1 deletion test/blackbox-tests/test-cases/misc/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
$ dune runtest --display short
diff alias runtest
diff alias runtest
diff alias runtest

Testing multiline commands in cram tests:
$ cat <<EOF
Expand Down
1 change: 1 addition & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/cannot-infer/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(rule (action (system "something")))
1 change: 1 addition & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/cannot-infer/dune-project
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(lang dune 1.11)
7 changes: 7 additions & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/long-form/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
(rule
(action (write-file b "It should work as well!")))

(alias
(name infer)
(deps b)
(action (cat %{deps})))
1 change: 1 addition & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/long-form/dune-project
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(lang dune 1.11)
1 change: 1 addition & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/no-target/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(rule (action (echo "something")))
1 change: 1 addition & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/no-target/dune-project
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(lang dune 1.11)
33 changes: 33 additions & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
When a rule target can be be inferred from the rule action, the target or targets field can be omitted.
This works with the short form of the rule stanza:

$ dune build @infer --root short-form
Entering directory 'short-form'
It worked!

But should work with the long form as well:

$ dune build @infer --root long-form
Entering directory 'long-form'
It should work as well!

When an action has no targets, an helpful error message is displayed:

$ dune build --root no-target
Entering directory 'no-target'
File "dune", line 1, characters 0-34:
1 | (rule (action (echo "something")))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Rule has no targets specified
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rgrinberg, FWIW, I don't think this is "cannot be inferred", this is "can be inferred to be empty", which is a different situation. If the intention is to make something opaque to dune, you do need an external command. (I'm not saying one test is more important or better than the other, just that they test different things)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Did you mean to mention @NathanReb however? I'd also prefer a message like dune was not able to infer any targets and none were manually specified.

Copy link
Collaborator

@aalekseyev aalekseyev Aug 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I meant to mention you because you suggested echo was an appropriate replacement for "bash". I don't think "none were manually specified" is useful in this situation because that's misleading: specifying targets won't help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I added the test case to showcase that the error message could be improved. I'll add a different test for the no-target case and use bash in this one again.

I'll take a look at how to improve the error message here!

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. So indeed we should have 2 messages:

  • No targets were specified
  • Dune did not infer any targets

@NathanReb would you like to improve dune to distinguish between these?

Copy link
Member

Choose a reason for hiding this comment

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

@NathanReb there's still no need to use bash btw. Use system.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the test case but from a very quick look it seems like it's going to take a tiny bit more effort to distinguish those two error cases so feel free to merge as it if you need to.

I'll work on that separately!

[1]

When the rule action targets cannot be inferred by dune, we should make it explicit
in the error message:

$ dune build --root cannot-infer
Entering directory 'cannot-infer'
File "dune", line 1, characters 0-36:
1 | (rule (action (system "something")))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Rule has no targets specified
[1]
6 changes: 6 additions & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/short-form/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
(rule (write-file b "It worked!"))

(alias
(name infer)
(deps b)
(action (cat %{deps})))
1 change: 1 addition & 0 deletions test/blackbox-tests/test-cases/rule-target-inferrence/short-form/dune-project
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(lang dune 1.11)