Variables for artifacts #2417

nojb · 2019-07-16T21:35:04Z

This PR implements #2416, closes #2416, closes #1630, closes #1544

nojb · 2019-07-16T21:37:42Z

So far the following are working:

%{EXT:MOD} used inside a dune file where EXT is cmo, cmx or cmi, and MOD is a module name, and
%{EXT:LIB} used inside a dune file where EXT is cma or cmxa and LIB is a module name

Next I will try to implement virtual targets to be used in the command line. This will make the whole thing very easy to test as well.

nojb · 2019-07-20T20:14:00Z

This is ready for review.

In order to keep it as simple as possible (both for the user as for the implementation), I made a series of simplifications with respect to the discussion in #2416, which I find fit nicely with the purpose of these variables and the fact that they are intended for advanced users:

The variables do not care if the object being referred to belongs to a different project or if it is private, you are free to refer to modules and/or libraries in arbitrary directories of the workspace.
Each library is referred to by its local name, not the public one, and the variables resolve to paths in the build context (_build/default/...), not the installation one.
The error handling in the case the module/library does not exist is a bit unsatisfactory (see the tests for examples): the reason is that current expansion API does not allow to propagate an error that occurs during expansion of a macro. At first glance it looks like improving on this would require some non-trivial refactoring.

One other interesting change is that I now use the parser for the dependency specification to parse the command line (in the non-alias case), opening the door to dune build '(glob_files *.foo)', etc. But all this is left for a later PR.

nojb · 2019-07-20T20:16:38Z

test/blackbox-tests/test-cases/variables-for-artifacts/run.t

+  26 | (alias
+  27 |  (name t3)
+  28 |  (deps %{cmo:xxxx}))
+  Error: No rule found for xxxx.cmo


When the object being referred to (here, a module named xxxx in the current directory), the macro expands anyway to a fictitious filename (here, xxxx.cmo) which then fails but with a message that is a bit misleading and does not mention the initial macro.

The problem is that it is not currently possible (as far as I can see) to propagate an error that occurs during expansion of a macro.

Do you mean to propagate the location?

nervermind, I see what you mean. Currently, the best is to error out while expanding the macro

Actually, yh, we do need to allow reporting proper error when expanding macros. The result of expand_var is actually a bit misleading. We return Error <expansion> for things that need to be expanded later. Instead of returning a result, we should return something like this:

type expanded = Static of Value.t list | Dynamic of Pform.Expansion.t | Error of User_message.t

I am trying to see how far the error case needs to be propagated inside the existing infrastructure.

Does it sound reasonable that the result of String_with_vars.expand would be a (Value.t list, User_message.t) result or maybe even (Value.t, User_message.t) result list ?

That seems right. We could also use the Or_exn.t type here since we use it for the same purpose in other places. Not too sure which of the two types you propose is better though.

Thanks for the hint, will look into it.

I think this is now fixed, thanks.

test/blackbox-tests/test-cases/variables-for-artifacts/run.t

src/expander.ml

nojb · 2019-07-21T04:59:00Z

bin/target.ml

+              (Dune.Dir_contents.artifacts (Dune.Dir_contents.get sctx ~dir)) name
+          in
+          let expander = Dune.Expander.set_lookup_module expander ~lookup_module in
+          let expander = Dune.Expander.set_lookup_library expander ~lookup_library in


It would be better to set these lookup functions inside the function Super_context.expander but Dir_contents depends on Super_context so there is a dependency issue.

ghost · 2019-07-23T12:08:59Z

Could you add some doc in the manual for this feature?

ghost · 2019-07-25T15:46:48Z

bin/target.ml

+            Dune.Dune_file.Dep_conf.decode
+        in
+        Dune_lang.Decoder.parse parser Univ_map.empty
+          (Dune_lang.parse_string ~fname:"command line" ~mode:Dune_lang.Parser.Mode.Single s)


Could you move this parsing into a cmdliner Arg.conv value? i.e. like the path value in bin/arg.ml. The idea would be to have:

val Arg.dep : Dune.Dune_file.Dep_conf.t Arg.conv

Then the error would be reported by cmdliner while parsing the command line, which seems nicer to me.

Since the parsing reports User_error.E exception, you will need to extract the error message from the User_message.t value. You can use this (from .../sexp_tests.ml) for that:

let extract_error_message (msg : User_message.t) = Format.asprintf "%a" Pp.render_ignore_tags (User_message.pp { msg with loc = None }) |> String.drop_prefix ~prefix:"Error: " |> Option.value_exn |> String.trim

Good idea, will do.

This is now done.

nojb · 2019-08-01T11:37:55Z

Could you add some doc in the manual for this feature?

Done. I think all other issues have been addressed so this is again ready for review :)

ghost · 2019-08-01T15:43:21Z

bin/arg.ml

+          match parse_alias s with
+          | None -> x
+          | Some (true, s) -> alias_rec s
+          | Some (false, s) -> alias s


I believe this function is parsing (file @foo) as Alias_rec "foo". We should probably call parse_alias before

You are right, I had missed that; will fix.

This is fixed now, thanks.

nojb · 2019-08-14T09:38:17Z

Okay, this is good enough to merge. One thing that I'm still questioning is the usefulness of this Obj_dir.Module.Single.t type. First of all, is it really that much better than a tuple? But really, is it even necessary? We can always go from module name to stanza (exe/library) and from that we can construct the object directory. This isn't essential, but I'm usually not a fan of storing stuff that we can easily calculate it. Especially if it's inexpensive.

I agree; I removed Obj_dir.Module.Single and switched to a tuple instead.

nojb · 2019-08-14T09:39:11Z

Planning to merge this after CI passes.

nojb · 2019-08-14T11:56:39Z

Actually, there is still an outstanding issue : currently these variables cannot be used in the action field of rule. The issue is that rule is interpreted early as a side effect of computing the Dir_contents.t in order to extract their targets. But I need to have the Dir_contents.t already computed in order to expand the variables.

I don't think there is any fundamental problem here, as the variables do not declare targets so it is "just" a matter of delaying their expansion until after the Dir_contents.t has been computed. But it looks like this will require some refactoring.

I am looking int this. If it looks like it requires some work, maybe we can merge this as-is, since it already adds some new functionality (command-line convenience targets), and then do the remaining work on a follow-up PR.

nojb · 2019-08-14T19:40:49Z

@diml @rgrinberg I would appreciate your opinion on the last commit ([RFC] user rules...).

The idea is to separate the computation of the targets of rule from the installation of the corresponding rule in the context. This allows passing different expanders at each point and allows to use the variables in this PR in the action field of rule (as explained in the previous note, the issue here is that in order to expand these variables we need access to the full Dir_contents.t, but the targets of the rule are needed while building this value).

Right now the I am duplicating the setup of the build arrow for rule, which is not satisfactory. Do you see how to memoize to avoid the duplicated work? Or perhaps a different approach altogether?

ghost · 2019-08-19T22:39:31Z

Would it be enough to make the expansion of the artifact variables dynamic? That should break the cycle

nojb · 2019-08-26T05:41:58Z

Would it be enough to make the expansion of the artifact variables dynamic? That should break the cycle

Thanks for the hint! I gave it a try (commit "Expand artifact variables dynamically") and now it works, but am not 100% sure about the implementation. Could you take a look and let me know if it looks like it is in the right direction?

rgrinberg · 2019-08-26T11:48:27Z

test/blackbox-tests/test-cases/variables-for-artifacts/dune

+;; (rule
+;;  (targets my2.cmxs)
+;;  (deps %{cmx:x2})
+;;  (action (run %{ocamlopt} -shared -o %{targets} %{cmxa:plugin} %{cmx:x3})))


You might as well make this a separate failing test I believe. Should be quite useful down the line and this way we won't forget to uncomment it if someone adds support

Good idea, will do.

rgrinberg · 2019-08-26T12:17:50Z

I approved because I don't think it's a good idea to delay this PR further. We can resolve whatever issues come up in separate PR's. Let's just make sure the test suite is extensive.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb · 2019-08-26T20:11:10Z

I approved because I don't think it's a good idea to delay this PR further. We can resolve whatever issues come up in separate PR's. Let's just make sure the test suite is extensive.

OK, planning to merge once the CI passes. Thanks!

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb · 2019-08-28T14:20:36Z

Merged. Thanks everyone for the help!

ghost · 2019-09-02T08:36:55Z

Thanks for the hint! I gave it a try (commit "Expand artifact variables dynamically") and now it works, but am not 100% sure about the implementation. Could you take a look and let me know if it looks like it is in the right direction?

I just had a quick look and it looks good to me!

nojb force-pushed the variables_for_artifacts branch 2 times, most recently from 1af1042 to b94fdfe Compare July 17, 2019 10:29

nojb changed the title ~~Variables for artifacts~~ [WIP] Variables for artifacts Jul 17, 2019

nojb force-pushed the variables_for_artifacts branch 3 times, most recently from b391dd2 to ef1c19b Compare July 20, 2019 05:28

nojb requested a review from a user July 20, 2019 16:37

nojb force-pushed the variables_for_artifacts branch from 0fd1944 to 07e1c38 Compare July 20, 2019 19:48

nojb changed the title ~~[WIP] Variables for artifacts~~ Variables for artifacts Jul 20, 2019

nojb commented Jul 20, 2019

View reviewed changes

test/blackbox-tests/test-cases/variables-for-artifacts/run.t Outdated Show resolved Hide resolved

nojb commented Jul 20, 2019

View reviewed changes

test/blackbox-tests/test-cases/variables-for-artifacts/run.t Outdated Show resolved Hide resolved

nojb commented Jul 21, 2019

View reviewed changes

test/blackbox-tests/test-cases/variables-for-artifacts/run.t Outdated Show resolved Hide resolved

nojb commented Jul 21, 2019

View reviewed changes

src/expander.ml Outdated Show resolved Hide resolved

nojb commented Jul 21, 2019

View reviewed changes

nojb force-pushed the variables_for_artifacts branch from b27af32 to b93868d Compare July 24, 2019 20:18

ghost reviewed Jul 25, 2019

View reviewed changes

nojb force-pushed the variables_for_artifacts branch from b93868d to ea3ef79 Compare July 25, 2019 20:19

bobot mentioned this pull request Jul 29, 2019

Library dependencies in aliases #2470

Closed

nojb force-pushed the variables_for_artifacts branch from ea3ef79 to ef9badb Compare July 29, 2019 16:21

nojb changed the title ~~Variables for artifacts~~ [WIP] Variables for artifacts Jul 29, 2019

nojb force-pushed the variables_for_artifacts branch from ef9badb to d062055 Compare July 31, 2019 19:51

nojb requested review from emillon and rgrinberg as code owners July 31, 2019 21:16

nojb force-pushed the variables_for_artifacts branch from 70c31f6 to d54b5af Compare August 1, 2019 11:08

ghost suggested changes Aug 1, 2019

View reviewed changes

nojb force-pushed the variables_for_artifacts branch from 84cc293 to dffa1f8 Compare August 14, 2019 09:44

nojb mentioned this pull request Aug 14, 2019

A way to express including a cmxa into a cmxs? #1544

Closed

nojb force-pushed the variables_for_artifacts branch from 1988562 to fc09300 Compare August 26, 2019 05:32

rgrinberg reviewed Aug 26, 2019

View reviewed changes

rgrinberg approved these changes Aug 26, 2019

View reviewed changes

nojb force-pushed the variables_for_artifacts branch 2 times, most recently from 61f2c86 to f73e157 Compare August 26, 2019 13:16

nojb added 2 commits August 26, 2019 21:40

Add plumbing to report macro expansion errors

8659ac7

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Dir_contents: track all artifacts (libs and exes)

8ee8be8

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb force-pushed the variables_for_artifacts branch 2 times, most recently from cbd9d9a to 38fe098 Compare August 26, 2019 19:55

nojb added 2 commits August 26, 2019 22:03

Add artifact variables %{<kind>:<path>}

25b196a

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Expand artifact variables dynamically

79192c1

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb force-pushed the variables_for_artifacts branch from 38fe098 to 737e6e7 Compare August 26, 2019 20:03

nojb added 2 commits August 26, 2019 22:37

Add test and doc

728c3ad

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

ocamlformat

a50ae68

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb force-pushed the variables_for_artifacts branch from 737e6e7 to a50ae68 Compare August 26, 2019 20:37

Merge branch 'master' into variables_for_artifacts

b758e84

nojb merged commit 2166934 into ocaml:master Aug 28, 2019

gasche mentioned this pull request Mar 27, 2021

confused by Dune foreign_libraries doc: how to define a C library used by C stubs? #4409

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variables for artifacts #2417

Variables for artifacts #2417

nojb commented Jul 16, 2019 •

edited

Loading

nojb commented Jul 16, 2019

nojb commented Jul 20, 2019

nojb Jul 20, 2019

ghost Jul 23, 2019

ghost Jul 23, 2019

ghost Jul 23, 2019

nojb Jul 28, 2019

ghost Jul 29, 2019

nojb Jul 29, 2019

nojb Jul 31, 2019

nojb Jul 21, 2019

ghost commented Jul 23, 2019

ghost Jul 25, 2019

nojb Jul 25, 2019

nojb Aug 1, 2019

nojb commented Aug 1, 2019

ghost Aug 1, 2019

nojb Aug 1, 2019

nojb Aug 1, 2019

nojb commented Aug 14, 2019

nojb commented Aug 14, 2019

nojb commented Aug 14, 2019

nojb commented Aug 14, 2019

ghost commented Aug 19, 2019

nojb commented Aug 26, 2019

rgrinberg Aug 26, 2019

nojb Aug 26, 2019

rgrinberg commented Aug 26, 2019

nojb commented Aug 26, 2019

nojb commented Aug 28, 2019

ghost commented Sep 2, 2019

Variables for artifacts #2417

Variables for artifacts #2417

Conversation

nojb commented Jul 16, 2019 • edited Loading

nojb commented Jul 16, 2019

nojb commented Jul 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb commented Aug 14, 2019

nojb commented Aug 14, 2019

nojb commented Aug 14, 2019

nojb commented Aug 14, 2019

ghost commented Aug 19, 2019

nojb commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Aug 26, 2019

nojb commented Aug 26, 2019

nojb commented Aug 28, 2019

ghost commented Sep 2, 2019

nojb commented Jul 16, 2019 •

edited

Loading