Add root and build context to dune describe output #6136

zpyemont · 2022-09-07T21:22:13Z

For ROTOR we would like to have access to the original source files, not their locations within the build directory. To get this, we need the build context and the workspace root.

This is an updated PR of #3834 and #6092, the latter of which I closed by mistake (sorry).

In this PR the build prefix and the path to the workspace root are added the the beginning of the output of dune describe.

@esope, would you be able to review this updated version? Thanks in advance.

esope

Added some suggested changes so that the code is more uniform, and requested some comments in the code.

esope · 2022-09-08T08:14:31Z

bin/describe.ml

+    type t = Path.t
+
+    let[@warning "-27"] map_path (_ : t) ~f =
+      Path.external_ (Path.External.of_string "/")


This was intended to be a map function. I was expecting let map_path (x: t) ~f = f t.
Then, you must change the rename_path function that is used during sanitization so that it sanitizes your path (maybe by looking for the workspace root as a prefix?). Also, a better sanitized output could be "WORKSPACE_ROOT" rather than "/", for example.

esope · 2022-09-08T08:15:10Z

bin/describe.ml

+  module BuildCtxt = struct
+    type t = Path.t
+
+    let[@warning "-27"] map_path t ~f = t


Same remark here as for lines 176-177.

esope · 2022-09-08T08:16:49Z

bin/describe.ml


    (** Conversion to the [Dyn.t] type *)
    let to_dyn options : t -> Dyn.t = function
      | Executables exe_descr ->
        Variant ("executables", [ Exe.to_dyn options exe_descr ])
      | Library lib_descr ->
        Variant ("library", [ Lib.to_dyn options lib_descr ])
+      | Root root -> Dyn.pair Dyn.string Root.to_dyn ("root", root)


I would prefer something similar to the previous cases, like Variant ("root", [ Root.to_dyn root ])

esope · 2022-09-08T08:17:37Z

bin/describe.ml


    (** Conversion to the [Dyn.t] type *)
    let to_dyn options : t -> Dyn.t = function
      | Executables exe_descr ->
        Variant ("executables", [ Exe.to_dyn options exe_descr ])
      | Library lib_descr ->
        Variant ("library", [ Lib.to_dyn options lib_descr ])
+      | Root root -> Dyn.pair Dyn.string Root.to_dyn ("root", root)
+      | BuildCtxt build_ctxt ->
+        Dyn.pair Dyn.string BuildCtxt.to_dyn ("build_context", build_ctxt)


Same here: I would suggest Variant ("build_context", [ BuildCtxt.to_dyn build_ctxt ])

esope · 2022-09-08T08:19:21Z

bin/describe.ml

  (** Description of items: executables, or libraries *)
  module Item = struct
    type t =
      | Executables of Exe.t
      | Library of Lib.t
+      | Root of Root.t
+      | BuildCtxt of BuildCtxt.t


In the to_dyn conversion on line 213, you use the tag name "build_context", so I would suggest that you use the same name for the variant constructor, i.e. Build_context.

esope · 2022-09-08T08:21:18Z

bin/describe.ml

@@ -425,6 +449,11 @@ module Crawl = struct
    source_path_is_in_dirs dirs
      (Path.drop_build_context_exn @@ Lib_info.best_src_dir @@ Lib.info lib)

+  let root () = Descr.Item.Root Path.root


Please add a comment to describe what this function computes.

esope · 2022-09-08T08:21:23Z

bin/describe.ml

@@ -425,6 +449,11 @@ module Crawl = struct
    source_path_is_in_dirs dirs
      (Path.drop_build_context_exn @@ Lib_info.best_src_dir @@ Lib.info lib)

+  let root () = Descr.Item.Root Path.root
+
+  let build_ctxt (context : Context.t) : Descr.Item.t =


Please add a comment to describe what this function computes.

esope · 2022-09-08T08:25:27Z

bin/describe.ml

-    exes @ libs
+    let root = root () in
+    let build_ctxt = build_ctxt context in
+    (root :: build_ctxt :: exes) @ libs


I would suggest root :: build_ctxt :: (exes @ libs), that computes the same result, but saves two recursive calls to the list concatenation function.

esope · 2022-09-08T08:29:05Z

bin/describe.ml

@@ -170,23 +170,47 @@ module Descr = struct
        ]
  end

+  module Root = struct


Please add a comment at the beginning of this module (similar to the comments for its brother modules).

esope · 2022-09-08T08:29:16Z

bin/describe.ml

+    let to_dyn t = Dyn.string (Path.to_absolute_filename t)
+  end
+
+  module BuildCtxt = struct


Please add a comment at the beginning of this module (similar to the comments for its brother modules).

reubenrowe · 2022-09-15T14:02:04Z

We have addressed the code review comments now.

esope

Looks good to me. Thanks!

esope · 2022-09-19T10:11:54Z

Can you please add an entry to CHANGES.md?

reubenrowe · 2022-09-19T19:21:13Z

Done @esope

esope · 2022-09-20T06:05:42Z

Thanks. This is fine for me. I guess we need yet somebody else's input. @rgrinberg ?

esope · 2022-10-09T12:27:15Z

@rgrinberg can we make some progress on this PR ?

rgrinberg · 2022-10-09T19:15:10Z

As a reviewer, you can formally approve the PR when you deem it's ready.

Add root and build context to dune describe output Signed-off-by: Zak Pyemont <zakpyemont@gmail.com>

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0~alpha1) CHANGES: - Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg) - Add a `runtime_deps` field in the `cinaps` stanza to specify runtime dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg) - Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg) - Extend dune describe to include the root path of the workspace and the relative path to the build directory. (ocaml/dune#6136, @reubenrowe) - Allow dune describe workspace to accept directories as arguments. The provided directories restrict the worskpace description to those directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope) - Add a terminal persistence mode that attempts to clear the terminal history. It is enabled by setting terminal persistence to `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg) - Disallow generating targets in sub direcories in inferred rules. The check to forbid this was accidentally done only for manually specified targets (ocaml/dune#6031, @rgrinberg) - Do not ignore rules marked `(promote (until-clean))` when `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon) - Dune no longer considers .aux files as targets during Coq compilation. This means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter) - Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg) - Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056, @rgrinberg) - Introduce a `dirs` field in the `install` stanza to install entire directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg) - Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg) - Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes ocaml/dune#5945, @rgrinberg) - Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg) - Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine) - Add an `(include <file>)` term to the `include_dirs` field for adding directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993, @gridbugs) - Support `(extra_objects ...)` field in `(executable ...)` and `(library ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs) - Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb) - Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114, fixes ocaml/dune#6103, @emillon) - odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes ocaml/dune#1117, @emillon) - dune install: copy files in an atomic way (ocaml/dune#6150, @emillon) - Add `%{coq:...}` macro for accessing data about the configuration about Coq. For instance `%{coq:version}` (ocaml/dune#6049, @Alizter) - update vendored copy of cmdliner to 1.1.1. This improves the built-in documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon, ocaml/dune#6169, @shonfeder) - The test suite for Coq now requires Coq >= 8.16 due to changes in the plugin loading mechanism upstream (which now uses findlib). - Starting with Coq build language 0.6, theories can be built without importing Coq's standard library by including `(stdlib no)`. (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek) - on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, fixes ocaml/dune#5650, @emillon) - Added an (aliases ...) field to the (rules ...) stanza which allows the specification of multiple aliases per rule (ocaml/dune#6194, @Alizter)

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) CHANGES: - macOS: Handle unknown fsevents without crashing (ocaml/dune#6217, @rgrinberg) - Enable file watching on MacOS SDK < 10.13. (ocaml/dune#6218, @rgrinberg) - Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg) - Add a `runtime_deps` field in the `cinaps` stanza to specify runtime dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg) - Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg) - Extend dune describe to include the root path of the workspace and the relative path to the build directory. (ocaml/dune#6136, @reubenrowe) - Allow dune describe workspace to accept directories as arguments. The provided directories restrict the worskpace description to those directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope) - Add a terminal persistence mode that attempts to clear the terminal history. It is enabled by setting terminal persistence to `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg) - Disallow generating targets in sub direcories in inferred rules. The check to forbid this was accidentally done only for manually specified targets (ocaml/dune#6031, @rgrinberg) - Do not ignore rules marked `(promote (until-clean))` when `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon) - Dune no longer considers .aux files as targets during Coq compilation. This means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter) - Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg) - Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056, @rgrinberg) - Introduce a `dirs` field in the `install` stanza to install entire directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg) - Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg) - Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes ocaml/dune#5945, @rgrinberg) - Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg) - Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine) - Add an `(include <file>)` term to the `include_dirs` field for adding directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993, @gridbugs) - Support `(extra_objects ...)` field in `(executable ...)` and `(library ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs) - Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb) - Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114, fixes ocaml/dune#6103, @emillon) - odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes ocaml/dune#1117, @emillon) - dune install: copy files in an atomic way (ocaml/dune#6150, @emillon) - Add `%{coq:...}` macro for accessing data about the configuration about Coq. For instance `%{coq:version}` (ocaml/dune#6049, @Alizter) - update vendored copy of cmdliner to 1.1.1. This improves the built-in documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon, ocaml/dune#6169, @shonfeder) - The test suite for Coq now requires Coq >= 8.16 due to changes in the plugin loading mechanism upstream (which now uses `Findlib`). - Starting with Coq build language 0.6, theories can be built without importing Coq's standard library by including `(stdlib no)`. (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek) - on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, ocaml/dune#6231, fixes ocaml/dune#5650, fixes ocaml/dune#6226, @emillon) - Added an (aliases ...) field to the (rules ...) stanza which allows the specification of multiple aliases per rule (ocaml/dune#6194, @Alizter) - The `(coq.theory ...)` stanza will now ensure that for each declared `(plugin ...)`, the `META` file for it is built before calling `coqdep`. This enables the use of the new `Findlib`-based loading method in Coq 8.16; however as of Coq 8.16.0, Coq itself has some bugs preventing this to work yet. (ocaml/dune#6167 , workarounds ocaml/dune#5767, @ejgallego) - Allow include statement in install stanza (ocaml/dune#6139, fixes ocaml/dune#256, @gridbugs) - Handle CSI n K code in ANSI escape codes from commands. (ocaml/dune#6214, fixes ocaml/dune#5528, @emillon) - Add a new experimental feature `mode_specific_stubs` that allows the specification of different flags and sources for foreign stubs depending on the build mode (ocaml/dune#5649, @voodoos)

rgrinberg requested a review from esope September 8, 2022 00:13

esope suggested changes Sep 8, 2022

View reviewed changes

zpyemont requested a review from esope September 15, 2022 14:03

esope approved these changes Sep 17, 2022

View reviewed changes

rgrinberg added this to the 3.5.0 milestone Oct 9, 2022

rgrinberg force-pushed the main branch from 0cbe848 to 81f38d5 Compare October 9, 2022 19:17

feature: dune describe fields

3645b10

Add root and build context to dune describe output Signed-off-by: Zak Pyemont <zakpyemont@gmail.com>

rgrinberg force-pushed the main branch from 81f38d5 to 3645b10 Compare October 9, 2022 21:10

rgrinberg merged commit 0dc164e into ocaml:main Oct 9, 2022

emillon mentioned this pull request Oct 19, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) ocaml/opam-repository#22317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add root and build context to dune describe output #6136

Add root and build context to dune describe output #6136

zpyemont commented Sep 7, 2022

esope left a comment

esope Sep 8, 2022

esope Sep 8, 2022

esope Sep 8, 2022

esope Sep 8, 2022

esope Sep 8, 2022

esope Sep 8, 2022

esope Sep 8, 2022

esope Sep 8, 2022

esope Sep 8, 2022

esope Sep 8, 2022

reubenrowe commented Sep 15, 2022

esope left a comment

esope commented Sep 19, 2022

reubenrowe commented Sep 19, 2022

esope commented Sep 20, 2022

esope commented Oct 9, 2022

rgrinberg commented Oct 9, 2022

Add root and build context to dune describe output #6136

Add root and build context to dune describe output #6136

Conversation

zpyemont commented Sep 7, 2022

esope left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reubenrowe commented Sep 15, 2022

esope left a comment

Choose a reason for hiding this comment

esope commented Sep 19, 2022

reubenrowe commented Sep 19, 2022

esope commented Sep 20, 2022

esope commented Oct 9, 2022

rgrinberg commented Oct 9, 2022