Add support for loading libraries from toplevel #2952

mbernat · 2019-12-05T15:12:05Z

Start work on loading dune libraries from toplevel
Finish draft version of toplevel-init-file
Add toplevel loading script
Add install stanza for dune.mlt
Remove dependency on Unix in the toplevel script
Add documentation for toplevel integration
Add a very basic test for toplevel-init-file

mbernat · 2019-12-05T15:25:56Z

Hi!
As per @diml's suggestion, I'm publishing this PR now just to get the ball rolling. I'm already able to use it to load my libraries in toplevels, but I'm sure there'll be issues since I'm new both to OCaml and even more so to Dune. Happy to work with you to address any problems.

bin/dune

ghost · 2019-12-10T08:26:25Z

doc/toplevel-integration.rst

+
+Second, to enhance usability, dune also provides a toplevel script, which does the above
+manual work for you. To use it, make sure to have `topfind` available in your toplevel by
+invoking `#use "topfind";;`. Afterwards you can run `#use "dune";;` and your


Do we actually need topfind for this? At the moment we only generate #directory and load directives and these are available by default.

My bad, I thought the Toplevel module comes from topfind, but apparently it's available by default. I'll update the docs.

test/blackbox-tests/test-cases/toplevel-integration/run.t

ghost · 2019-12-10T08:32:09Z

test/blackbox-tests/test-cases/toplevel-integration/run.t

+  | (name test)
+  #directory "$TESTCASE_ROOT/_build/default/.test.objs/byte";;
+  #directory "$TESTCASE_ROOT/_build/default/.test.objs/native";;
+  #load "$TESTCASE_ROOT/_build/default/test.cma";;


Could you add a test testing the whole story? i.e. doing #use "..." from a toplevel? And also one testing an error case, just to see what the error looks like. You can do the following:

$ ocaml -stdin <<EOF > #use "..." > ...use code that was just loaded... > EOF

Okay, I have several questions here:

Even outside of tests, doing just #use "dune.top" doesn't work in ocaml(it can't find the file), but it does in utop. Is ocaml using OCAML_TOPLEVEL_PATH or does it resolve paths in some other fashion? I wasn't able to figure this out by googling and I only use plain ocaml rarely. But of course, we want to support it, that's the whole point of this PR! :)

When running the tests, OCAML_TOPLEVEL_PATH still points to my user-local directory. Am I supposed to run export OCAML_TOPLEVEL_PATH="_build/install/default/lib/toplevel/dune.top" or is there a nicer way to refer to the local installation we're testing?

What error case do you have in mind?

I did a bit of research and right now, packages that install toplevel scripts such as ocamlfind or down install them in both <libdir>/toplevel and <libdir>/ocaml. The latter is immediately available in the toplevel.

OCAML_TOPLEVEL_PATH is only used by utop and not by the vanilla toplevel. However, since 4.08.0, there is a new variable OCAMLTOP_INCLUDE_PATH that is supported natively. I opened a ticket to discuss setting this variable in opam: ocaml/opam-repository#15547. Let's see what happens there. At worse we could install dune.top in <libdir>/ocaml as well.

On the dune side, dune already extends a few environment variable so that when running dune or other tools inside dune, various tools will look for things in _build/install in addition to things installed globally. We should do this for OCAMLTOP_INCLUDE_PATH as well. More precisely, you can look for extend_var "OCAMLPATH" in src/dune/context.ml and add the following line: extend_var "OCAMLTOP_INCLUDE_PATH" (Path.relative local_lib_path "toplevel").

Then, in the test #use "dune.top" will work out of the box with OCaml >= 4.08. We run the testsuite with 4.09 in the CI so having the test only work with OCaml >= 4.08 is fine.

Regarding the error case, I was thinking of a case where compilation fails. For instance, what does the user see if one of the ml file that dune tries to compile doesn't compile? Or if a dune file is invalid.

Thanks for all this great info and sorry about my (usual) late response!

Here's what I've done for now. Since there are still several unknowns, I've also had to make several questionable choices.

I've added extend_var as you suggested.

I've also installed dune.top into <libdir>/ocaml, since we can't rely on the opam PR being finished anytime soon (right?).

I've put back the documentation bit that people need to do #use "topfind" first. Indeed, it turns out that without that I don't have Toplevel module available in dune.top (I missed this because I had #use "topfind" in my .ocamlinit). Do you think it's possible to remove the dependency on #use "topfind" or, even better, on the Toplevel module?

I've added a few tests, including failures. However, the failures expose compilation internals and I'm not sure whether the tests won't break randomly: https://github.com/mbernat/dune/blob/toplevel/test/blackbox-tests/test-cases/toplevel-integration/run.t#L35

Please let me know what you think about this.

In case I don't get around to this again for a week, I wish you a very Merry Christmas and thanks again for all your help! 🎄

Looking at those errors more closely, they actually reference my local paths, so they're definitely wrong.

How do you handle test errors containing local paths? And more importantly, what kind of error behavior should we expect from running dune toplevel-init-file and dune.top?

Just back from holidays. Thanks, hope you had a nice Christmas too!

For 2, that seems reasonable.

For 3, we can do the same as the topfind script, i.e. temporary add the +compiler-libs directory:

#directory "+compiler-libs";; ... #remove_directory "+compiler-libs";;

For 4, I was surprised by this output as normally Dune doesn't print the full compilation command line if it detects that the error messages already points to a file, i.e. is of the form File "xxx", .... After a bit of digging, I found that we always print the full compilation command when dune is run inside dune because of this in src/dune/config.ml:

let show_full_command_on_error () = inside_dune || inside_ci || !Clflags.always_show_command_line

@rgrinberg, I don't remember the details. Was the inside_dune || part intentional?

ghost · 2019-12-10T08:36:07Z

Thanks! I left a few comments but overall it looks good to me. I can think of one issue related to findlib: if the user does #use "dune" in a running toplevel where some libraries have already been loaded via #require, then these libraries are going to be loaded again which will result in failures or odd behaviours

The solution for this involves detecting if findlib is available and if yes issuing #require directives rather than #directory/#load ones. However, I suggest that we do this in a separate PR in order to keep this one simple.

mbernat · 2019-12-13T16:16:28Z

Thank you! I fixed what I could and asked about what I couldn't.

I agree about the #require problem, let's postpone it. Just to make sure I understand it properly: dune also tracks the findlib dependencies and we'd simply generate a #require for each of them?

Btw, what is the relation between dune libraries (as defined by the dune files) and findlib libraries? Are they essentially equivalent?

ghost · 2019-12-16T13:46:36Z

bin/dune

@@ -23,6 +23,11 @@
 (files
  (dune.exe as dune)))

+(install
+  (section lib_root)


Just realised that there is a toplevel section available. We might as well use it and replace (dune.mlt as toplevel/dune) by (dune.mlt as dune.top)

Also, while we are it we could name the file dune.top directly in the source tree.

Makes sense, done.

ghost · 2019-12-16T14:15:49Z

I agree about the #require problem, let's postpone it. Just to make sure I understand it properly: dune also tracks the findlib dependencies and we'd simply generate a #require for each of them?

Yes, that's the idea. But only if findlib is loaded in the toplevel. We could detect that by checking in the #require directive is available in dune.top.

Btw, what is the relation between dune libraries (as defined by the dune files) and findlib libraries? Are they essentially equivalent?

Any dune library with a public name will be installed on the system as a findlib library. Findlib libraries can be consumed in dune projects the same way dune libraries can. So in short, findlib libraries are essentially equivalent to dune public libraries.

On the other hand, dune private libraries are not visible to findlib. Although, it would be nice to change that so you could use dune private libraries the same way you use findlib libraries. Indeed, this would be especially useful here. But this is quite a bit more work.

ghost · 2019-12-16T14:16:35Z

BTW, I don't see your latest changes. Did you push them?

rgrinberg · 2019-12-21T17:03:57Z

test/blackbox-tests/test-cases/toplevel-integration/run.t

+  > EOF
+  $ cat >dune <<EOF
+  > (library
+  >     (name test)


1 space indentation please

rgrinberg · 2019-12-21T17:04:36Z

src/dune/toplevel.ml

+let print_toplevel_init_file ~include_paths ~files_to_load =
+  let includes = Path.Set.to_list include_paths in
+  List.iter includes ~f:(fun p ->
+    print_endline("#directory \"" ^ Path.to_absolute_filename p ^ "\";;"));


Please format your code with ocamlformat. $ make fmt does taht for you

rgrinberg · 2019-12-21T17:05:35Z

bin/toplevel_init_file.ml

+
+let link_deps link ~lib_config =
+  List.map link ~f:(fun t -> Dune.Lib.link_deps t Dune.Link_mode.Byte lib_config)
+  |> List.flatten


Can you use List.concat_map?

In fact, this function becomes unnecessary if it's just a call to concat_map

rgrinberg · 2019-12-21T17:06:10Z

bin/toplevel_init_file.ml

+    let* setup = Import.Main.setup common in
+    let sctx =
+      Dune.Context_name.Map.find setup.scontexts Dune.Context_name.default
+      |> Option.value_exn


This should take a context argument like $ dune utop.

rgrinberg · 2019-12-21T17:07:29Z

src/dune/utop.mli

@@ -8,4 +8,6 @@ val utop_exe : string

 val is_utop_dir : Path.Build.t -> bool

+val libs_under_dir : Super_context.t -> db:Lib.DB.t -> dir:Import.Path.t -> Lib.L.t


Let's not use Import in the mli. It should just be Path.t and Stdune be opened.

rgrinberg · 2019-12-21T17:08:47Z

bin/toplevel_init_file.ml

+    let files_to_load =
+      List.filter files ~f:(fun p ->
+        match Path.extension p with
+          | ".cma" | ".cmo" -> true


Should be Mode.compiled_lib_ext and Cm_kind.ext.

rgrinberg

I've left some minor comments, but overall, the feature looks fine. Thank you for your hard work.

Was it discussed to have these .init scripts be generated by targets? It seems like it makes them usable in a few more contexts. Such as the ability to be auto updated in watch mode, and the ability to launch multiple top levels.

rgrinberg · 2019-12-22T05:48:03Z

Please sign your commits by the way. See CONTRIBUTING.md for how

mbernat · 2019-12-23T11:41:15Z

Please sign your commits by the way. See CONTRIBUTING.md for how

All my commits are signed. Am I missing something?

rgrinberg · 2019-12-23T12:00:24Z

Hmm well the DCO bot isn’t happy. I’ll have a closer look later I guess

ghost · 2020-01-06T13:56:04Z

Was it discussed to have these .init scripts be generated by targets? It seems like it makes them usable in a few more contexts. Such as the ability to be auto updated in watch mode, and the ability to launch multiple top levels.

I thought about it, but it would slightly slow down all builds systematically while the current method doesn't disturb existing builds. We can also launch multiple top levels. For watch mode, I'm not too sure what would be different. I mean, in any case the user has to manually restart the toplevel. And when they do so, they'll get the latest updates.

ghost · 2020-01-15T16:04:58Z

@mbernat your commits are GPG signed. This is fine, but this is not what we require for Dune contributions. We need the DCO sign-off.

ghost · 2020-03-19T16:13:15Z

@mbernat gentle ping, the signature is the last bit we need to merge this work :)

BTW, I recently added support for #use_output "<command>" in the OCaml toplevel, and I'm thinking to use this for this feature, rather than installing a dune.top file at the right place. It will keep dune more self-contained.

mbernat · 2020-03-22T16:58:17Z

@diml Sorry about the long silence, I have had very little energy for contributions because of a new job these past few months.

I signed my commit. The DCO bot is happy, but the AppVeyor isn't. IIRC there were issues with the tests that we haven't resolved.

ghost · 2020-03-24T11:13:29Z

No problem, hope your new job is going well! Thanks for signing the commits, I'll do the finishing touches. We are doing some other work on the toplevel so I'd like to merge this soon.

ghost · 2020-03-24T12:36:17Z

I rebased the feature and replaced the dune.top file by using the new #use_output command of OCaml 4.11. The new usage is:

# #use_output "dune toplevel-init-file"

ghost · 2020-03-24T16:52:04Z

The command name looks a bit long to me now, I'm wondering if it shouldn't be just top:

# #use_output "dune top"

What do others think?

* Start work on loading dune libraries from toplevel * Finish draft version of toplevel-init-file * Add toplevel loading script * Add install stanza for dune.top * Remove dependency on Unix in the toplevel script * Add documentation for toplevel integration * Add a very basic test for toplevel-init-file Signed-off-by: Marek Bernat <marek.bernat@gmail.com>

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

ghost · 2020-03-26T13:52:13Z

Alright, I renamed the command dune top. Merging now. Thanks again @mbernat for your time working on this feature!

Add a "dune top" command one is expected to call via: # #use_output "dune top";; In the toplevel. This command should work in any toplevel.

@diml

…lugin, dune-private-libs and dune-glob (2.5.0) CHANGES: - Add a `--release` option meaning the same as `-p` but without the package filtering. This is useful for custom `dune` invocation in opam files where we don't want `-p` (ocaml/dune#3260, @diml) - Fix a bug introduced in 2.4.0 causing `.bc` programs to be built with `-custom` by default (ocaml/dune#3269, fixes ocaml/dune#3262, @diml) - Allow contexts to be defined with local switches in workspace files (ocaml/dune#3265, fix ocaml/dune#3264, @rgrinberg) - Delay expansion errors until the rule is used to build something (ocaml/dune#3261, fix ocaml/dune#3252, @rgrinberg, @diml) - [coq] Support for theory dependencies and compositional builds using new field `(theories ...)` (ocaml/dune#2053, @ejgallego, @rgrinberg) - From now on, each version of a syntax extension must be explicitely tied to a minimum version of the dune language. Inconsistent versions in a `dune-project` will trigger a warning for version <=2.4 and an error for versions >2.4 of the dune language. (ocaml/dune#3270, fixes ocaml/dune#2957, @voodoos) - [coq] Bump coq lang version to 0.2. New coq features presented this release require this version of the coq lang. (ocaml/dune#3283, @ejgallego) - Prevent installation of public executables disabled using the `enabled_if` field. Installation will now simply skip such executables instead of raising an error. (ocaml/dune#3195, @voodoos) - `dune upgrade` will now try to upgrade projects using versions <2.0 to version 2.0 of the dune language. (ocaml/dune#3174, @voodoos) - Add a `top` command to integrate dune with any toplevel, not just utop. It is meant to be used with the new `#use_output` directive of OCaml 4.11 (ocaml/dune#2952, @mbernat, @diml) - Allow per-package `version` in generated `opam` files (ocaml/dune#3287, @toots) - [coq] Introduce the `coq.extraction` stanza. It can be used to extract OCaml sources (ocaml/dune#3299, fixes ocaml/dune#2178, @rgrinberg) - Load ppx rewriters in dune utop and add pps field to toplevel stanza. Ppx extensions will now be usable in the toplevel (ocaml/dune#3266, fixes ocaml/dune#346, @stephanieyou) - Add a `(subdir ..)` stanza to allow evaluating stanzas in sub directories. (ocaml/dune#3268, @rgrinberg) - Fix a bug preventing one from running inline tests in multiple modes (ocaml/dune#3352, @diml) - Allow the use of the `%{profile}` variable in the `enabled_if` field of the library stanza. (ocaml/dune#3344, @mrmr1993) - Allow the use of `%{ocaml_version}` variable in `enabled_if` field of the library stanza. (ocaml/dune#3339, @voodoos) - Fix dune build freezing on MacOS when cache is enabled. (ocaml/dune#3249, fixes #ocaml/dune#2973, @artempyanykh)

mbernat requested review from emillon, nojb and rgrinberg as code owners December 5, 2019 15:12

mbernat mentioned this pull request Dec 5, 2019

Add support for dune rtop alongside dune utop #2694

Closed

ghost suggested changes Dec 10, 2019

View reviewed changes

ghost suggested changes Dec 16, 2019

View reviewed changes

mbernat force-pushed the toplevel branch from ff4a9a2 to 52f17f4 Compare December 21, 2019 16:17

rgrinberg reviewed Dec 21, 2019

View reviewed changes

rgrinberg requested changes Dec 21, 2019

View reviewed changes

ghost mentioned this pull request Feb 5, 2020

Add support for #use_output "<command>" (was: #use "| <command>") ocaml/ocaml#9283

Merged

mbernat force-pushed the toplevel branch from 91043e2 to a28a6bb Compare March 22, 2020 16:48

ghost force-pushed the toplevel branch from a28a6bb to 889e539 Compare March 24, 2020 12:33

mbernat and others added 4 commits March 26, 2020 10:59

Rely on #use_command rather than installing a dune.top file

40c51a3

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Update changelog

f5b4635

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

toplevel-init-file -> top

8bdbc3c

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

ghost force-pushed the toplevel branch from 889e539 to 8bdbc3c Compare March 26, 2020 11:00

ghost merged commit 026a4e7 into ocaml:master Mar 26, 2020

rgrinberg mentioned this pull request Apr 10, 2020

[new release] dune-build-info, dune, dune-configurator, dune-action-plugin, dune-private-libs and dune-glob (2.5.0) ocaml/opam-repository#16170

Merged

rgrinberg linked an issue Sep 7, 2021 that may be closed by this pull request

Add support for dune rtop alongside dune utop #2694

Closed

This pull request was closed.

		@@ -8,4 +8,6 @@ val utop_exe : string

		val is_utop_dir : Path.Build.t -> bool

		val libs_under_dir : Super_context.t -> db:Lib.DB.t -> dir:Import.Path.t -> Lib.L.t

Add support for loading libraries from toplevel #2952

Add support for loading libraries from toplevel #2952

Conversation

mbernat commented Dec 5, 2019

mbernat commented Dec 5, 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

ghost commented Dec 10, 2019

mbernat commented Dec 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 16, 2019

ghost commented Dec 16, 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

rgrinberg left a comment

Choose a reason for hiding this comment

rgrinberg commented Dec 22, 2019

mbernat commented Dec 23, 2019

rgrinberg commented Dec 23, 2019

ghost commented Jan 6, 2020

ghost commented Jan 15, 2020

ghost commented Mar 19, 2020

mbernat commented Mar 22, 2020

ghost commented Mar 24, 2020

ghost commented Mar 24, 2020

ghost commented Mar 24, 2020 • edited by ghost Loading

ghost commented Mar 26, 2020

ghost commented Mar 24, 2020 •

edited by ghost

Loading