Rename path variables in dune files

[emillon]

• ${path:file} -> %{dep:file}
• ${path-no-dep:file} -> %{path:file}

See #842.

I moved the path-no-dep test that was in misc and expanded it a bit - I don't think that the path: (with dep) part was directly tested, so I added a small test as well.

[rgrinberg]

Implementation looks good, but I wonder if it might be worth it to complicate things a little more for the sake of better error messages. Concretely, we'd like to handle the following cases better:

• Using %{dep:..} in jbuild files
• Using %{path-no-dep:..} in dune files.

[emillon]

Sure, let's do this!

[emillon]

Done - I tried to simplify a bit the match by removing the guards, but it's now full of booleans. I'm open to suggestions to make this more readable.

[ghost]

@emillon what about doing this in the main pattern match? i.e.:

| Pair ("dep", s) -> if not new_syntax then fail ...

[emillon]

I tried this, but that requires duplicating the happy path (e.g. Some (path_exp (Path.relative dir s))). But if I extract a function for these, that should look OK. I'll try that.

[ghost]

BTW, I'm having second thoughts about renaming path-no-dep to path; it means that {path:x} exists in both the old and new syntax but mean something different, which might make things confusing for users. In particular, it might be tempting to replace ${path:x} by %{path:x}. Should we keep keep path-no-dep as it for now and only rename it in dune 2?

[emillon]

Yep that looks better!

[emillon]

About your comment - yes I agree that reusing the same piece of syntax is a bit dangerous. Can we avoid path altogether? What is the use case for using ${path-no-dep:x} or %{path:x} (the ones that do not add the dependency) rather than x? Capture the current directory?

[ghost]

The idea is that the payload of ${path-no-dep:...} is relative to the directory of the jbuild file while its expansion is relative to the cwd when running the command. So for instance (chdir ${ROOT} (run echo ${path-no-dep:f})) in src/jbuild expands to: (chdir .. (run echo src/f))

[ghost]

We could possibly replace it by just %{dir} or similar that would be the equivalent of ${path-no-dep:.}, then we'd do: %{dir}/f rather than %{path:f}

[rgrinberg]

A more radical approach might be just adding a %{no-dep:x:...}, where x can be path, exe, bin, etc.

[ghost]

A more radical approach might be just adding a %{no-dep:x:...}, where x can be path, exe, bin, etc.

That would be more generic indeed, however in most of these case the file is always a dependency.

While I remember needing this in the past, I can't find a single occurrence of path-no-dep in the dune universe. I can't really think of a valid use case for it either. So I suggest that we indeed get rid of it. Although adding a variable to refer to the directory of the dune file seems good to me, just in case someone was indeed using ${path-no-dep:...}.

[emillon]

A disadvantage of using %{dir}/f is that we cannot check that the corresponding path is still within the _build directory (but we can reject %{path:../../..}). I'm trying to thinking of a better name.

[ghost]

That's true, but we can't check it either if the user writes a plain path without variables

[emillon]

That's fair! I'll leave that part out and remove path-no-dep in a separate PR.