Allow variables in (targets ...) #1301
Conversation
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
nojb
force-pushed
the
vars_in_targets
branch
from
5fb71ba to
f73e81d
Compare
src/simple_rules.ml
Outdated
| "%s does not denote a file in the current directory" fn; | ||
| Path.relative ~error_loc:loc dir fn | ||
| in | ||
| Static (List.map fns ~f) |
There was a problem hiding this comment.
One unfortunate thing about this code is that it won't be able to extend variables into multiple targets. That's a bit unnatural, especially given how deps work. I suggest that we use:
SC.expand_vars ~mode:String_with_vars.Mode.Many
do the error checking like you've done, and then use Value.to_path.
There was a problem hiding this comment.
I will look into it. But just to make sure I understand: currently there are no "list" variables which are static enough to be used in the targets field, right?
There was a problem hiding this comment.
Technically, there are a few. Things like flags or binareis defined in Ocaml_config.t. Not the most useful things, but why not keep things consistent.
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
src/simple_rules.ml
Outdated
| Path.relative ~error_loc:loc dir fn | ||
| ) (SC.expand_vars_string_list sctx ~scope ~dir fn) | ||
| in | ||
| Static (List.concat_map ~f fns) |
There was a problem hiding this comment.
This will work, but it's a bit weird to stringify the paths and then turn them back to paths again.
It would be best to just expand things to Value.t and then pattern match on it in the List.map. You can handle paths just by letting them through, strings can have the same validation + conversion, and the rest we should error out on.
This is what I meant in my original comment. So if it's not clear, I can make the little tweak myself.
There was a problem hiding this comment.
But I would still need to check that Path values denote a file in the current directory, right? In which am not sure this really would simplify much.
There was a problem hiding this comment.
You could check that on the Path.t value by checking that Path.parent path = Some dir. This avoids converting back and forth between strings and paths.
There was a problem hiding this comment.
Thanks, I added a commit with this suggestion.
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
nojb
force-pushed
the
vars_in_targets
branch
from
629ddaa to
5bc3037
Compare
|
Thanks, I merged the PR with a change entry. |
CHANGES: - Support colors on Windows (ocaml/dune#1290, @diml) - Allow `dune.configurator` and `base` to be used together (ocaml/dune#1291, fix ocaml/dune#1167, @diml) - Support interrupting and restarting builds on file changes (ocaml/dune#1246, @kodek16) - Fix findlib-dynload support with byte mode only (ocaml/dune#1295, @bobot) - Make `dune rules -m` output a valid makefile (ocaml/dune#1293, @diml) - Expand variables in `(targets ..)` field (ocaml/dune#1301, ocaml/dune#1320, fix ocaml/dune#1189, @nojb, @rgrinberg, @diml) - Fix a race condition on Windows that was introduced in 1.2.0 (ocaml/dune#1304, fix ocaml/dune#1303, @diml) - Fix the generation of .merlin files to account for private modules (@rgrinberg, fix ocaml/dune#1314) - Exclude the local opam switch directory (`_opam`) from the list of watched directories (ocaml/dune#1315, @dysinger) - Fix compilation of the module generated for `findlib.dynload` (ocaml/dune#1317, fix ocaml/dune#1310, @diml)
CHANGES: - Support colors on Windows (ocaml/dune#1290, @diml) - Allow `dune.configurator` and `base` to be used together (ocaml/dune#1291, fix ocaml/dune#1167, @diml) - Support interrupting and restarting builds on file changes (ocaml/dune#1246, @kodek16) - Fix findlib-dynload support with byte mode only (ocaml/dune#1295, @bobot) - Make `dune rules -m` output a valid makefile (ocaml/dune#1293, @diml) - Expand variables in `(targets ..)` field (ocaml/dune#1301, ocaml/dune#1320, fix ocaml/dune#1189, @nojb, @rgrinberg, @diml) - Fix a race condition on Windows that was introduced in 1.2.0 (ocaml/dune#1304, fix ocaml/dune#1303, @diml) - Fix the generation of .merlin files to account for private modules (@rgrinberg, fix ocaml/dune#1314) - Exclude the local opam switch directory (`_opam`) from the list of watched directories (ocaml/dune#1315, @dysinger) - Fix compilation of the module generated for `findlib.dynload` (ocaml/dune#1317, fix ocaml/dune#1310, @diml)
CHANGES: - Support colors on Windows (ocaml/dune#1290, @diml) - Allow `dune.configurator` and `base` to be used together (ocaml/dune#1291, fix ocaml/dune#1167, @diml) - Support interrupting and restarting builds on file changes (ocaml/dune#1246, @kodek16) - Fix findlib-dynload support with byte mode only (ocaml/dune#1295, @bobot) - Make `dune rules -m` output a valid makefile (ocaml/dune#1293, @diml) - Expand variables in `(targets ..)` field (ocaml/dune#1301, ocaml/dune#1320, fix ocaml/dune#1189, @nojb, @rgrinberg, @diml) - Fix a race condition on Windows that was introduced in 1.2.0 (ocaml/dune#1304, fix ocaml/dune#1303, @diml) - Fix the generation of .merlin files to account for private modules (@rgrinberg, fix ocaml/dune#1314) - Exclude the local opam switch directory (`_opam`) from the list of watched directories (ocaml/dune#1315, @dysinger) - Fix compilation of the module generated for `findlib.dynload` (ocaml/dune#1317, fix ocaml/dune#1310, @diml) - Lift restriction on `copy_files` and `copy_files#` stanzas that files to be copied should be in a subdirectory of the current directory. (ocaml/dune#1323, fix ocaml/dune#911, @nojb)
A first try at #1189, allowing to use variables in
(targets ...).