[WIP] Expand variables in `install` stanzas -- take 2 #1354

mseri · 2018-09-28T11:37:05Z

This PR is very preliminary and reflects my work towards fixing issue #690. I used #962 as the starting point and I have opened this to track the work and see if I am heading to the right directory.

To generate String_with_vars for now I used make_text, but in the discussion in #962 you recommended virt_text. Here maybe I misunderstood something.

The build now fails in https://github.com/mseri/dune/blob/814f0d52dbc4feda11aa9ad5605f41a8e592e71f/src/artifacts.ml#L19
There I see that we do lots of operations on src and dst seen as normal string types. If I understand correctly I need the super context to be able to expand them, so I was thinking to change the get the values out beforehand in https://github.com/mseri/dune/blob/814f0d52dbc4feda11aa9ad5605f41a8e592e71f/src/super_context.ml#L547
Is that correct or I am missing something?

ghost · 2018-09-28T22:29:12Z

Yes, that's correct. However, there is a simpler way to achieve this: instead of going through all the dune files in Artifacts.create, we can simply call Build_system.targets_of ~dir:bin_dir. This needs to be done lazily. It should simplify the code quite a bit.

mseri · 2018-10-08T10:21:23Z

I am fairly sure the code is incorrect in multiuple places right now, but I am trying to figure out why it is not yet even parsing correctly the sexp:

./boot.exe
File "editor-integration/emacs/dune", line 4, characters 8-44:
4 |  (files (dune.el as emacs/site-lisp/dune.el)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Unexpected list
Hint: dune files require less parentheses than jbuild files.
If you just converted this file from a jbuild file, try removing these parentheses.

I thought that 7813792#diff-8caffdb0f0d1de5eb40eee1dc4ece1f9R1182 was doing the right thing

mseri · 2018-10-08T14:37:35Z

I found out the issue: boot.exe was not being rebuilt (and make clean was broken due to missing dune_main.exe), so it had always tested an old wrong version of the code.

Using Build_system.targets_of in a naive way, made it ~~segfault~~ overflow immediately. I still need to understand where the lazyness should go here to avoid it, but I did not yet spend much time thinking about it to be honest.

ghost · 2018-10-08T14:54:26Z

Ok, I'm having a look at the code. The segfault is probably be due to a stack overflow

ghost · 2018-10-08T14:55:14Z

src/artifacts.ml

-              String.Map.add acc key in_bin_dir)
-        | _ ->
-          local_bins))
+  (*Build_system.targets_of build_system ~dir:bin_dir


The is that the local_bins field should be a Lazy.t, and this while computation should be under a lazy (...).

Even if this is lazy, as soon as it is used, it blows up

ghost · 2018-10-08T14:55:56Z

src/dune_file.ml

-    | Atom (_, A src) -> junk >>| fun () -> { src; dst = None }
-    | List (_, [Atom (_, A src); Atom (_, A "as"); Atom (_, A dst)]) ->
-      junk >>> return { src; dst = Some dst }
+    | Atom (_, A _) ->


This should be: Atom _ | Quoted_string _ | Template _

Updated, thanks!

mseri · 2018-10-08T14:57:14Z

It is, I can post you what it looks like if you want. I think the most meaningful part of the overflow is

Description:
("Build_system.get_collector called on closed directory"
 (dir (In_build_dir install/default/bin))
 (load_dir_stack ()))
Backtrace:
Raised at file "src/stdune/exn.ml", line 32, characters 5-10
Called from file "src/build_system.ml", line 1577, characters 18-47
Called from file "src/install_rules.ml", line 240, characters 6-58
Called from file "list.ml", line 88, characters 20-23
Called from file "list.ml", line 88, characters 32-39
Called from file "list.ml", line 88, characters 32-39
[...]
Called from file "list.ml", line 88, characters 32-39
Called from file "src/install_rules.ml", line 275, characters 6-70
Called from file "map.ml", line 295, characters 20-25
Called from file "src/install_rules.ml", line 363, characters 4-19
Called from file "src/gen_rules.ml", line 166, characters 4-25
Called from file "map.ml", line 295, characters 20-25
Called from file "src/gen_rules.ml", line 248, characters 2-60
Called from file "src/fiber/fiber.ml", line 111, characters 22-27
Called from file "src/fiber/fiber.ml", line 299, characters 6-13
[... thanks for this part, I could not stop laughing ...]

mseri · 2018-10-08T15:01:09Z

The overflow above is generated using the lazy version of the code. I have pushed that code as well, I hope it can help to understand what is wrong

ghost · 2018-10-08T15:53:33Z

I believe it's the expansion of dst in install_rules.ml. It might create a path at the wrong place. Could you try printing what dst expands to? I believe the fix will be using ~dir:Path.root for the expansion of dst.

mseri · 2018-10-08T16:17:13Z

Thanks for the analysis. I'll have a look and let you know!

mseri · 2018-10-08T16:45:09Z

The generated values before the failure are:

src: dst -> dune.el: emacs/site-lisp/dune.el
src: dst -> dune-flymake.el: emacs/site-lisp/dune-flymake.el
src: dst -> dune.1: <None>
src: dst -> dune-config.5: <None>
src: dst -> dune-build.1: <None>
src: dst -> dune-clean.1: <None>
src: dst -> dune-exec.1: <None>
src: dst -> dune-external-lib-deps.1: <None>
src: dst -> dune-help.1: <None>
src: dst -> dune-install.1: <None>
src: dst -> dune-installed-libraries.1: <None>
src: dst -> dune-printenv.1: <None>
src: dst -> dune-promote.1: <None>
src: dst -> dune-rules.1: <None>
src: dst -> dune-runtest.1: <None>
src: dst -> dune-subst.1: <None>
src: dst -> dune-uninstall.1: <None>
src: dst -> dune-unstable-fmt.1: <None>
src: dst -> dune-utop.1: <None>
src: dst -> main_dune.exe: dune
src: dst -> main_jbuilder.exe: jbuilder

Changing dirto Path.root in the expander produces the same output and fails in the same way. But I have a starting point on what to look for now, thanks

mseri · 2018-10-08T16:57:45Z

Doing the same in local_install_rules I get the attached output: local_install_rules.txt. Interestingly that one fails before printing the rule for main_jbuilder.exe (very early, at entry 408 of 409).

Also I can declutter a lot the stack by using a rev_map there, and with that the output does no longer look like a stack overflow.

ghost · 2018-10-08T17:49:04Z

I added the following code inside the lazy (...) in artifacts.ml:

      Printf.eprintf "---------------\nforced:%s\n===============\n%!"
        (Printexc.get_callstack 32 |> Printexc.raw_backtrace_to_string);

and got the following backtrace:

Raised by primitive operation at file "src/artifacts.ml", line 15, characters 9-34
Called from file "camlinternalLazy.ml", line 27, characters 17-27
Called from file "src/artifacts.ml", line 33, characters 26-51
Called from file "src/odoc.ml", line 88, characters 13-84
[...]

Looking at odoc.ml:88, the problem is clear: odoc is calling Super_context.resolve_program too soon. We could make this value a Lazy.t as well but we might have this issue elsewhere and it is hard to debug.

We can solve the issue by making sure that Install_rules.init is called before. To do that, move the following code in src/gen_rules.ml:

    let module Install_rules =
      Install_rules.Gen(P)
    in
    Install_rules.init ();

to just before the following line:

    let module M = Gen(struct let sctx = sctx end) in

mseri · 2018-10-09T12:20:43Z

Unfortunately, moving the install rules init in Gen_rules produced other even more confusing failures in some packages. I am trying to see how bad it is to fix the old state of things instead.

One big source of failures was the sometimes unnecessary call to dir_contents, already removing that seem to have reduced the errors drastically: d85f166

mseri · 2018-10-09T12:47:53Z

Independently of how one puts it, there seems to be an issue when we need to expand strings with vars too early and this is called:

https://github.com/ocaml/dune/blob/master/src/super_context.ml#L319-L326

This happens for example when looking for cppo during the compilation of js_of_ocaml-compiler

mseri · 2018-10-09T13:04:53Z

I have added a commit with the wrong behaviour of postponing looking in the local bins to after querying the context. I just want to see how far would the tests go with that (locally with the change it could compile every package I tried)

ghost · 2018-10-09T15:51:51Z

Hmm, I see it must be because Dir_contents causes a few rules to be evaluated. One possibility would be to split the part that setups the rule to install things in bin/ in install_rules.ml. For instance, we could interpret all the Install stanzas first. That should be enough to populate the _build/<context>/bin directory and interpreting the install stanza shouldn't cause a recursive loop.

mseri · 2018-10-23T08:12:25Z

I have been away without the laptop, this PR is still alive. I'll try to split the setup as you suggest. Thanks

rgrinberg · 2018-11-06T17:49:54Z

Ping @mseri

mseri · 2018-11-07T10:18:51Z

Thanks for the ping, I had some very busy days. I have rebased on the current master, fixed the merge issues and I am looking at the code changes. I think to make it work the setup of the install rules has to happen at init, but the rest has to be delayed. When I try to do that I incur in the issue that I am generating multiple install rules for package.install. How do you suggest to do the split? Just changing the order of the interpretation does not solve the issue

ghost · 2018-11-07T11:23:44Z

If you look at the function init_install in install_rules.ml,you should only process the Install { section = Bin; _ } stanzas at init time.

What you should do with these is create the Install.Entry.t values as currently done by the code in init_install, and them pass them through local_install_rules as done in install_file. You should them keep the result of local_install_rules for the second step, i.e. where the rest of the setup rules will be processed.

So essentially the process should be as follow (in pseudo-code):

let step1 () =
  let bin_entries, other_stanzas = List.partition_map stanzas_to_consider  ... in
  let bin_entries = local_install_rules bin_entries ... in
  (bin_entries, other_stanzas)

let step2 (bin_entries, other_stanzas) =
   let other_entries = ... in
   let entries = List.rev_append bin_entries other_entries in
   generate_install_file entries

ghost · 2018-11-07T11:25:40Z

step1 should be called before the let module M = Gen ..., and step2 should be called at the same place where Install_rules.init is called in master

mseri · 2018-11-08T10:30:06Z

Thanks! Indeed, this seems to work for all the packges I've tested, including the one that used to fail

mseri · 2018-11-08T10:45:13Z

I cannot reproduce appveyor's failure locally, I have also tried with an empty switch. But I don't have a Windows machine to try it.

ghost · 2018-11-08T11:09:59Z

src/artifacts.ml

+  lazy (
+    Build_system.targets_of build_system ~dir:bin_dir
+    |> Path.Set.fold ~init:String.Map.empty ~f:(fun path acc ->
+      let name = Filename.basename (Path.to_string path) in


If you keep the let key = if Sys.win32 ... in and use key instead of name in the following line, then the Windows error should go away. The idea is that the keys in the map are the executable names without the .exe, even on Windows.

Thanks. This regression seems fixed now!

ghost · 2018-11-08T11:25:36Z

src/dune_file.ml

+        (String_with_vars.decode >>= fun src ->
+         keyword "as" >>>
+         String_with_vars.decode >>= fun dst ->
+         return { src; dst = Some dst })


In general we try to produce helpful error messages when users use a new feature without upgrading the (lang dune ...) line in their dune-project file. In this case I suggest to replace the String_with_vars.decode by:

let%map is_atom = peek_exn >>| function Atom _ -> true | _ -> false and s = String_with_vars.decode and version = Syntax.get_exn Stanza.syntax in if not is_atom && version < (1, 6) then Syntax.Error.since (String_with_vars.loc s) Syntax.version (1, 6) ~what:(sprintf "Using %s here" (if String_with_vars.has_vars s then "variables" else "quoted strings")); s

and then bump the language version in stanza.ml to (1, 6).

You are right, I had seen a similar pattern in the file when I started and completely forgotten about it. Thanks

ghost · 2018-11-08T11:41:53Z

src/install_rules.ml

-  let init_install () =
-    let entries_per_package =
-      List.concat_map (SC.stanzas_to_consider_for_install sctx)
+  let preinit =


I find the following more descriptive:

Suggested change

let preinit =

let bin_entries, other_stanzas =

And I suppose init_install can just take () as argument and access these directly.

Indeed! That remained as an artifact of one of the intermediate changes.

ghost · 2018-11-08T11:43:49Z

src/gen_rules.ml

+    let module P = struct let sctx = sctx end in
+    (* We need to instantiate Install_rules earlier to avoid issues whenever
+     * Super_context is used too soon.
+     * See: https://github.com/ocaml/dune/pull/1354#issuecomment-427922592 *)


This comment looks good, but I would put it closer to the module Install_rules = Install_rules.Gen(P).

ghost · 2018-11-08T11:45:49Z

src/install_rules.ml

  let install_file (package : Package.t) entries =
-    let entries =
+    let install_paths =
+      Install.Section.Paths.make ~package:package.name ~destdir:Path.root ()


It seems that we are effectively calling this 3 times: once here and twice through process_entries. It'd be nice if we could call it only once, though it's probably not a big deal in practice sowe can keep the code as it if it's annoying to change.

I had thought about that and was not sure what would have been better. I refactored it out, it will be called once at the price of a lookup in the map and a call to Option.value_exn (that should be safe in this case as we are computing the install paths for all the packages). It can be extracted further instead of staying in the pre-init step but I think it is more consistent to keep it all contained

ghost · 2018-11-08T13:38:47Z

Thanks, this all looks good! Could you run dune runtest followed by dune promote and commit the result? (This is needed every time we bump the language version). Then it'll be ready to merge.

mseri · 2018-11-08T14:29:29Z

I was adding a test to check that the variables were working correctly. I can remove it if you think it's not robust enough or if it is not needed.

While doing that I also checked that the version error comes up nicely:

+  File "dune", line 8, characters 8-29:
+  8 |  (files %{read:man-pages.inc})
+              ^^^^^^^^^^^^^^^^^^^^^
+  Error: Using variables here is only available since version 1.6 of the dune language

ghost · 2018-11-08T14:48:23Z

Thanks, testing new features is always welcome 👍 Could you just remove the library and executable from the test? Just to keep the output short and focused on the feature to test.

ghost · 2018-11-08T14:49:20Z

test/blackbox-tests/test-cases/install-with-var/run.t

+  Installing install/man/man1/a-man-page.default.1
+  Installing install/man/man3/another-man-page.3
+
+If prefix is passed, the default for libdir is `$prefix/lib`:


The rest of this file shouldn't be necessary, the first test is enough to check that variable expansion works correctly.

mseri · 2018-11-08T15:08:35Z

Done, thanks! Should I cleanup the history or you want to keep it to make it easier to reconstruct what issues we had?

ghost · 2018-11-08T15:21:12Z

I prefer a shorter history, so if you could clean it that would be nice, thanks. But TBH, I'm not too fuzzy regarding the history, I'm happy with everything squashed into a single commit.

This requires adding lazyness to the sctx resolution of the documentation, and splitting the preparation of install stanzas in two parts: in the first step we only process binary install paths, to make sure that they are available when the rest of the dune file is processed and some of the variables are expanded, in the second step we finish processing all the other install stanzas and complete the of init the install targets. Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri · 2018-11-08T15:33:02Z

I have rebased into three commits. I think it is better this way as two of them where somewhat unrelated to the rest of the changes

ghost · 2018-11-08T15:43:38Z

Thanks again for this PR! It is a nice user feature and a nice simplification of the code handling executables.

mseri · 2018-11-08T15:50:00Z

Thank you very much for guiding me through it!

@mseri

CHANGES: - Expand variables in `install` stanzas (ocaml/dune#1354, @mseri) - Add predicate language support for specifying sub directories. This allows the use globs, set operations, and special values in specifying the sub directories used for the build. For example: `(dirs :standard \ lib*)` will use all directories except those that start with `lib`. (ocaml/dune#1517, ocaml/dune#1568, @rgrinberg) - Add `binaries` field to the `(env ..)` stanza. This field sets and overrides binaries for rules defined in a directory. (ocaml/dune#1521, @rgrinberg) - Fix a crash caused by using an extension in a project without dune-project file (ocaml/dune#1535, fix ocaml/dune#1529, @diml) - Allow `%{bin:..}`, `%{exe:..}`, and other static expansions in the `deps` field. (ocaml/dune#1155, fix ocaml/dune#1531, @rgrinberg) - Fix bad interaction between on-demand ppx rewriters and using multiple build contexts (ocaml/dune#1545, @diml) - Fix handling of installed .dune files when the backend is declared via a `dune` file (ocaml/dune#1551, fixes ocaml/dune#1549, @diml) - Add a `--stats` command line option to record resource usage (ocaml/dune#1543, @diml) - Fix `dune build @doc` deleting `highlight.pack.js` on rebuilds, after the first build (ocaml/dune#1557, @aantron). - Allow targets to be directories, which Dune will treat opaquely (ocaml/dune#1547, @jordwalke) - Support for OCaml 4.08: `List.t` is now provided by OCaml (ocaml/dune#1561, @ejgallego) - Exclude the local esy directory (`_esy`) from the list of watched directories (ocaml/dune#1578, @andreypopp) - Fix the output of `dune external-lib-deps` (ocaml/dune#1594, @diml) - Introduce `data_only_dirs` to replace `ignored_subdirs`. `ignored_subdirs` is deprecated since 1.6. (ocaml/dune#1590, @rgrinberg)

mseri requested a review from a user September 28, 2018 11:37

mseri requested review from emillon and rgrinberg as code owners September 28, 2018 11:37

ghost suggested changes Oct 8, 2018

View reviewed changes

ghost mentioned this pull request Nov 6, 2018

Add support for adding private binaries into scope #1521

Merged

2 tasks

ghost suggested changes Nov 8, 2018

View reviewed changes

ghost reviewed Nov 8, 2018

View reviewed changes

ghost approved these changes Nov 8, 2018

View reviewed changes

mseri added 3 commits November 8, 2018 16:27

stdune: simplify calls stack using tail recursive map

4a46bd9

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

Makefile: ensure boot.exe removal when running make clean

07bf4bf

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

ghost merged commit 73c63e5 into ocaml:master Nov 8, 2018

mseri deleted the issue690 branch November 8, 2018 15:50

ghost mentioned this pull request Nov 21, 2018

Introduce Local_package module #1554

Merged

rgrinberg mentioned this pull request Nov 29, 2018

[new release] dune (1.6.0) ocaml/opam-repository#13065

Merged

This pull request was closed.

[WIP] Expand variables in install stanzas -- take 2 #1354

[WIP] Expand variables in install stanzas -- take 2 #1354

Conversation

mseri commented Sep 28, 2018 • edited Loading

ghost commented Sep 28, 2018

mseri commented Oct 8, 2018 • edited Loading

mseri commented Oct 8, 2018 • edited Loading

ghost commented Oct 8, 2018

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

mseri commented Oct 8, 2018 • edited Loading

mseri commented Oct 8, 2018 • edited Loading

ghost commented Oct 8, 2018

mseri commented Oct 8, 2018

mseri commented Oct 8, 2018

mseri commented Oct 8, 2018 • edited Loading

ghost commented Oct 8, 2018

mseri commented Oct 9, 2018 • edited Loading

mseri commented Oct 9, 2018 • edited Loading

mseri commented Oct 9, 2018

ghost commented Oct 9, 2018 • edited by ghost Loading

mseri commented Oct 23, 2018 • edited Loading

rgrinberg commented Nov 6, 2018

mseri commented Nov 7, 2018

ghost commented Nov 7, 2018

ghost commented Nov 7, 2018

mseri commented Nov 8, 2018

mseri commented Nov 8, 2018 • edited Loading

ghost Nov 8, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Nov 8, 2018 • edited by ghost Loading

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 Nov 8, 2018 • edited by ghost Loading

mseri commented Nov 8, 2018

ghost commented Nov 8, 2018

Choose a reason for hiding this comment

mseri commented Nov 8, 2018

ghost commented Nov 8, 2018

mseri commented Nov 8, 2018

ghost commented Nov 8, 2018

mseri commented Nov 8, 2018

[WIP] Expand variables in `install` stanzas -- take 2 #1354

[WIP] Expand variables in `install` stanzas -- take 2 #1354

mseri commented Sep 28, 2018 •

edited

Loading

mseri commented Oct 8, 2018 •

edited

Loading

mseri commented Oct 8, 2018 •

edited

Loading

mseri commented Oct 8, 2018 •

edited

Loading

mseri commented Oct 8, 2018 •

edited

Loading

mseri commented Oct 8, 2018 •

edited

Loading

mseri commented Oct 9, 2018 •

edited

Loading

mseri commented Oct 9, 2018 •

edited

Loading

ghost commented Oct 9, 2018 •

edited by ghost

Loading

mseri commented Oct 23, 2018 •

edited

Loading

mseri commented Nov 8, 2018 •

edited

Loading

ghost Nov 8, 2018 •

edited by ghost

Loading

ghost Nov 8, 2018 •

edited by ghost

Loading

ghost commented Nov 8, 2018 •

edited by ghost

Loading