Add dune-project stanza, map_workspace_root, to control workspace map… #6988

richardlford · 2023-02-03T19:22:08Z

Add a "map_workspace_root" stanza to the dune-project file. If true, references to the workspace root directory in output files are mapped to "/workspace_root". If false, such references are not modified.
If missing, it defaults to true.

Note that when enabled, the debug search directories in the debug information produced by ocamlc are also mapped, with the result that ocamldebug cannot find the files.

Before this change mapping was being controlled by the Dune version number (>= 3.0), so the mapping was taking place (for this version of Dune), and ocamldebug was broken.

Thanks to @rgrinberg for pointing out the way to access the new stanza where I needed it.

Signed-off-by: Richard L Ford [email protected]

Fixes #6929.

rgrinberg · 2023-02-03T19:43:35Z

src/dune_engine/dune_project.ml

@@ -717,6 +728,9 @@ let parse ~dir ~lang ~file ~dir_status =
        and+ wrapped_executables =
          field_o_b "wrapped_executables"
            ~check:(Dune_lang.Syntax.since Stanza.syntax (1, 11))
+        and+ map_workspace_root =
+          field_o_b "map_workspace_root"
+            ~check:(Dune_lang.Syntax.since Stanza.syntax (3, 6))


Should be (3, 7)

@rgrinberg, I have pushed to correct this, and also in the documentation.

rgrinberg · 2023-02-03T19:44:08Z

The implementation looks right to me. @snowleopard can you take a glance at it as well?

richardlford · 2023-02-03T19:45:01Z

@rgrinberg, I forgot to include the sign-off message in my commit (did not read the contributing.md file before making the PR). I added it to the PR comment. Is that sufficient, or should I make a new PR with proper signoff?

rgrinberg · 2023-02-03T19:50:26Z

It's not sufficient, but there's no need for a new PR either. You can fix your commit with git commit --amend --sign-off and force push.

richardlford · 2023-02-03T19:56:11Z

It's not sufficient, but there's no need for a new PR either. You can fix your commit with git commit --amend --sign-off and force push.

OK. That seems to have worked.

rgrinberg · 2023-02-04T05:31:18Z

DCO's are still missing unfortunately

snowleopard · 2023-02-04T16:13:39Z

doc/dune-files.rst

+
+    (map_workspace_root <bool>)
+
+Starting with Dune 3.0, Dune does this mapping by default. However,


Should this be 3.6?

(Also, it seems like there is some duplication between this paragraph and the one above.)

Should this be 3.6?

I was basing 3.0 on the following code:

let add_workspace_root_to_build_path_prefix_map t = t.dune_version >= (3, 0)

which is the existing (before this PR) test for whether to map or not.

Actually, I see some ambiguity. There is the version of Dune itself, and there is the version as specified in the "(lang dune ...)" stanza in the dune-project file. Do we document the Dune version or the Dune language version? Or are they supposed to be consistent? If they are supposed to be consistent, then the above would imply that this mapping started at version 3.0 (I don't know the actual history).

Currently this PR, in the absence of a user-specified "map_workspace_root" stanza, uses the default given by:

let map_workspace_root_default ~(lang : Lang.Instance.t) = lang.version >= (3, 6)

which is not consistent with the behavior before this PR. So it seems this "3, 6" should be "3, 0". Do you agree?

(Also, it seems like there is some duplication between this paragraph and the one above.)

I have pushed a change to remove the redundancy.

snowleopard · 2023-02-04T16:25:00Z

The implementation looks right to me. @snowleopard can you take a glance at it as well?

I am confused. After this PR, we will be in a world where Dune is broken either because the bytecode executables built by it cannot be debugged, or because it leaks absolute paths everywhere. And the user will be able to choose one breakage or the other via the new configuration setting. But both options are bad! Well, I guess, one is worse than the other in some cases but still, this strikes me as a dubious solution.

rgrinberg · 2023-02-04T18:50:05Z

Yes, this "solution" is a compromise between two bad options. The user gets to choose between reproducible artifacts or a usable ocamldebug. Unfortunately this is the best we can do for now.

Perhaps if it was possible to tell ocamldebug how to interpret this fake workspace root at runtime somehow, we'd have a better workaround. @gasche does that sound reasonable?

gasche · 2023-02-04T22:56:03Z

In general, extending ocamldebug with new features sounds perfectly reasonable to me, but those features should ideally make sense / be pleasant in themselves, rather than feeling like dune-specific hack.

I don't understand the full details of the situation you are discussing, but from a distance it sounds like the feature you would propose naively would be "-workspace-root <path> replaces /workspace-root by <path> when interpreting paths", and that sounds a bit ad-hoc to me.

Naive question:

Why do we use /workspace-root instead of just nothing? If we used foo instead of /workspace-root/foo, would one of the existing ocamldebug options -cd <path> or -I <path> work for whatever is intended there? (see man ocamldebug for a description of those options)
Not really a question, but: I realize that I don't really know what a "workspace" is in dune parlance, and what the "workspace root" is. (I sort of assume that this is the subdirectory of _build that matches the layout of my project sources, and contains the sources and their build artifacts, but I'm not sure.) Where is this explained in the documentation?

christinerose

Looks great! Just a minor syntax suggestion and spelling format for bytecode.

doc/dune-files.rst

richardlford · 2023-02-06T17:27:10Z

DCO's are still missing unfortunately

@rgrinberg, if I look at the 2 commits above, they both have the signed-off message. Why is that not working? I could create a new squashed and up-to-date PR that is properly signed-off.

@rgrinberg, @snowleopard, @gasche: But before that, I'd like to investigate how much work it would be to modify ocamldebug (and ocamldebug.el and ocamlearlybird) to deal with either the "/workspace_root" mapping, or perhaps just using relative addresses. I'll post another response when I have more information.

I also wanted to comment that this is not the first ocamldebug breakage caused by Dune. (wrapped_executables true) can also cause problems. Ocamldebug itself handles it OK in the CLI, provided the user remembers to prefix "Dune__exe__" to the name of the user's name of the module, when requesting a breakpoint. But in the emacs interface, if you tried to set a breakpoint using the ocamldebug-break command (bound to C-x space), it will not work because it assumes the module name is derived from the source file name and does not know it needs to preface the file root with "Dune__exe__" to get the module name.

That, and the current issue, lead me to believe that the dune testing does not include testing that the OCaml debugger(s) work with the dune-built binaries. I will also investigate how hard it would be to add debug testing to Dune's testing.

Alizter · 2023-02-06T17:34:14Z

@richardlford I suspect because your first commit doesn't finish with the signed-off-by.

Alizter · 2023-02-06T18:02:11Z

@richardlford Your display name is:

Signed-off-by: Richard L Ford

however your have in your commits:

Signed-off-by: Richard L. Ford

richardlford · 2023-02-06T18:42:26Z

@richardlford I suspect because your first commit doesn't finish with the signed-off-by.

@Alizter, actually, I was just looking at the check link, and it has the message:

Commit sha: 5bd4b3d, Author: richardlford, Committer: richardlford; Expected "richardlford [[email protected]]", but got "Richard L Ford [[email protected]]"

I think when I created the PR my git user.name was "richardlford", but then I looked at the contributing guidelines where it asked for a real name, so I changed it to "Richard L. Ford". But as you observed, in my github profile my display name is "Richard L Ford". So how does DCO decide what to expect for the signoff? And does it matter whether it is on the last line?

Alizter · 2023-02-06T18:43:46Z

@richardlford I don't know the answers to those questions. When I commit, I don't write the sign off by hand but pass -S to git commit. That should give the correct sign off. Just rebase and amend each commit with -S.

richardlford · 2023-02-06T19:19:20Z

DCO seems to be working now.

richardlford · 2023-02-07T17:08:00Z

@rgrinberg, @snowleopard, @gasche: I just did another push to fix the last formatting problems. If you generally approve I think this is ready for a merge.

I do have some additional information on its impact.

The OCaml runtime itself ignores the "dirs" part of the debug information (see ocaml runtime/backtrace_byte.c, around line 475, in the ocaml5 sources).
ocamldebug does read the directories part of the debug information but appends them to the list of directories supplied by the "-I" option, and after that does not treat them specially. So one could provide "-I" options to make up for faulty debug directories (as a workaround for this mapping).
As part of this investigation I wrote a tool, ocamldumper, to dump the debug information. Temporarily I have it here. It is based on ocamlobjinfo, so later I'd like to do a PR to add this to that tool. This is what helped me to first diagnose the workspace mapping problem.
Using ocamldumper, I looked at the debug directories, not just for my small test case, but also for modules installed by opam and linked into my test case. I observed that there were indeed a lot of "dangling" paths in the debug information, e.g. references to the .opam-switch/build directory, that was used for the build, but that is not kept after the build completes.
A grep through the opam lib shows that there are already instances of "/workspace_root" in it.
The debug events themselves have the location information. The filename part is a path relative to the place where the build was taking place.

richardlford · 2023-02-08T15:06:42Z

@rgrinberg, @snowleopard, @gasche: Hi. I just clicked the button to rebase. I understand a project collaborator must enable a testing cycle. Is any other action on my part needed for this? Is this ready to merge, or do you think that ocamldebug and other tools should be fixed instead?

emillon

Just a minor tweak to the doc. This will also need an entry in the changelog. A test would also be nice but I'm not sure how easy it is to extract the list of directories that's embedded in the executable. And this is fairly low risk so this can go in 3.7.

doc/dune-files.rst

CHANGES.md

richardlford · 2023-02-08T20:56:38Z

@rgrinberg, @snowleopard, @gasche: I think I have responded to all your requests. There now is a CHANGES.md entry and two tests (for enabled or disabled). I think we are ready for another full test cycle.

Alizter · 2023-02-08T20:58:12Z

@richardlford I think it would be a good idea to squash and force push your commits into a single commit.

richardlford · 2023-02-08T20:59:48Z

@richardlford I think it would be a good idea to squash and force push your commits into a single commit.

I was just thinking about that. Is that something that is usually done automatically as an option when the merge is done, or is that usually done by the author?

Alizter · 2023-02-08T21:05:28Z

We have that option when merging, but it means the person merging (who won't be me) will have to remember to do it. I think its easier to squash it yourself.

richardlford · 2023-02-08T21:05:56Z

We have that option when merging, but it means the person merging (who won't be me) will have to remember to do it. I think its easier to squash it yourself.

OK. Will do.

richardlford · 2023-02-08T21:25:08Z

@rgrinberg, @snowleopard, @gasche, @Alizter: I have force-pushed a squashed version, just rebased to the main line.

richardlford · 2023-02-08T22:52:33Z

@rgrinberg, @snowleopard, @gasche, @Alizter: Hmm. Any idea why the new test (mapping enabled) would fail in two of the environments?

richardlford · 2023-02-08T23:45:53Z

I was able to reproduce the failures locally, though I'm not sure what has changed. Will investigate further.

richardlford · 2023-02-09T00:09:18Z

@rgrinberg, @snowleopard, @gasche, @Alizter: OK. Apparently, in some configurations, it is necessary to quote the "EOF" in order to get expansion to happen. I've added back the quotes (I had modeled my tests on path-rewriting.t test). It was passing and my test was not. The only difference was the quotes. I've squashed and force pushed again. Hopefully all will pass now.

richardlford · 2023-02-09T01:33:44Z

@rgrinberg, @snowleopard, @gasche, @Alizter: All checks passed. Is it ready for merge?

richardlford · 2023-02-09T11:23:06Z

@rgrinberg, @snowleopard, @gasche, @Alizter: I pushed the button to rebase again, to ready for merge.

emillon · 2023-02-09T14:52:46Z

@richardlford thanks!

…ping. Add a "map_workspace_root" stanza to the dune-project file. If true, references to the workspace root directory in output files are mapped to "/workspace_root". If false, such references are not modified. If missing, it defaults to true. Note that in the added tests, for some configurations quotes are needed around the "EOF" delimeter to get expansion. Note also that when enabled, the debug search directories in the debug information produced by ocamlc are also mapped, with the result that ocamldebug cannot find the files. Fixes ocaml#6929, provided user disables mapping. Signed-off-by: Richard L Ford <[email protected]> Co-authored-by: Christine Rose <[email protected]> Co-authored-by: Etienne Millon <[email protected]> Co-authored-by: Ali Caglayan <[email protected]>

@richardlford

…, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.0~alpha3) CHANGES: - Add map_workspace_root dune-project stanza to allow disabling of mapping of workspace root to /workspace_root. (ocaml/dune#6988, fixes ocaml/dune#6929, @richardlford)

@gridbugs

…, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.0) CHANGES: - Allow running `$ dune exec` in watch mode (with the `-w` flag). In watch mode, `$ dune exec` the executed binary whenever it is recompiled. (ocaml/dune#6966, @gridbugs) - `coqdep` is now called once per theory, instead of one time per Coq file. This should significantly speed up some builds, as `coqdep` startup time is often heavy (ocaml/dune#7048, @Alizter, @ejgallego) - Add `map_workspace_root` dune-project stanza to allow disabling of mapping of workspace root to `/workspace_root`. (ocaml/dune#6988, fixes ocaml/dune#6929, @richardlford) - Fix handling of support files generated by odoc. (ocaml/dune#6913, @jonludlam) - Fix parsing of OCaml errors that contain code excerpts with `...` in them. (ocaml/dune#7008, @rgrinberg) - Pre-emptively clear screen in watch mode (ocaml/dune#6987, fixes ocaml/dune#6884, @rgrinberg) - Fix cross compilation configuration when a context with targets is itself a host of another context (ocaml/dune#6958, fixes ocaml/dune#6843, @rgrinberg) - Fix parsing of the `<=` operator in *blang* expressions of `dune` files. Previously, the operator would be interpreted as `<`. (ocaml/dune#6928, @tatchi) - Fix `--trace-file` output. Dune now emits a single *complete* event for every executed process. Unterminated *async* events are no longer written. (ocaml/dune#6892, @rgrinberg) - Fix preprocessing with `staged_pps` (ocaml/dune#6748, fixes ocaml/dune#6644, @rgrinberg) - Use colored output with MDX when Dune colors are enabled. (ocaml/dune#6462, @MisterDA) - Make `dune describe workspace` return consistent dependencies for executables and for libraries. By default, compile-time dependencies towards PPX-rewriters are from now not taken into account (but runtime dependencies always are). Compile-time dependencies towards PPX-rewriters can be taken into account by providing the `--with-pps` flag. (ocaml/dune#6727, fixes ocaml/dune#6486, @esope) - Print missing newline after `$ dune exec`. (ocaml/dune#6821, fixes ocaml/dune#6700, @rgrinberg, @Alizter) - Fix binary corruption when installing or promoting in parallel (ocaml/dune#6669, fixes ocaml/dune#6668, @edwintorok) - Use colored output with GCC and Clang when compiling C stubs. The flag `-fdiagnostics-color=always` is added to the `:standard` set of flags. (ocaml/dune#4083, @MisterDA) - Fix the parsing of decimal and hexadecimal escape literals in `dune`, `dune-package`, and other dune s-expression based files (ocaml/dune#6710, @shym) - Report an error if `dune init ...` would create a "dune" file in a location which already contains a "dune" directory (ocaml/dune#6705, @gridbugs) - Fix the parsing of alerts. They will now show up in diagnostics correctly. (ocaml/dune#6678, @rginberg) - Fix the compilation of modules generated at link time when `implicit_transitive_deps` is enabled (ocaml/dune#6642, @rgrinberg) - Allow `$ dune utop` to load libraries defined in data only directories defined using `(subdir ..)` (ocaml/dune#6631, @rgrinberg) - Format dune files when they are named `dune-file`. This occurs when we enable the alternative file names project option. (ocaml/dune#6566, @rgrinberg) - Move `$ dune ocaml-merlin -dump-config=$dir` to `$ dune ocaml merlin dump-config $dir`. (ocaml/dune#6547, @rgrinberg) - Allow compilation rules to be impacted by `(env ..)` stanzas that modify the environment or set binaries. (ocaml/dune#6527, @rgrinberg) - Coq native mode is now automatically detected by Dune starting with Coq lang 0.7. `(mode native)` has been deprecated in favour of detection from the configuration of Coq. (ocaml/dune#6409, @Alizter) - Print "Leaving Directory" whenever "Entering Directory" is printed. (ocaml/dune#6419, fixes ocaml/dune#138, @cpitclaudel, @rgrinberg) - Allow `$ dune ocaml dump-dot-merlin` to run in watch mode. Also this command shouldn't print "Entering Directory" mesages. (ocaml/dune#6497, @rgrinberg) - `dune clean` should no longer fail under Windows due to the inability to remove the `.lock` file. Also, bring the implementation of the global lock under Windows closer to that of Unix. (ocaml/dune#6523, @nojb) - Remove "Entering Directory" messages for `$ dune install`. (ocaml/dune#6513, @rgrinberg) - Stop passing `-q` flag in `dune coq top`, which allows for `.coqrc` to be loaded. (ocaml/dune#6848, fixes ocaml/dune#6847, @Alizter) - Fix missing dependencies when detecting the kind of C compiler we're using (ocaml/dune#6610, fixes ocaml/dune#6415, @emillon) - Allow `(include_subdirs qualified)` for OCaml projects. (ocaml/dune#6594, fixes ocaml/dune#1084, @rgrinberg) - Accurately determine merlin configuration for all sources selected with `copy#` and `copy_files#`. The old heuristic of looking for a module in parent directories is removed (ocaml/dune#6594, @rgrinberg) - Fix inline tests with *js_of_ocaml* and whole program compilation mode enabled (ocaml/dune#6645, @hhugo) - Fix *js_of_ocaml* separate compilation rules when `--enable=effects` ,`--enable=use-js-string` or `--toplevel` is used. (ocaml/dune#6714, ocaml/dune#6828, ocaml/dune#6920, @hhugo) - Fix *js_of_ocaml* separate compilation in presence of linkall (ocaml/dune#6832, ocaml/dune#6916, @hhugo) - Remove spurious build dir created when running `dune init proj ...` (ocaml/dune#6707, fixes ocaml/dune#5429, @gridbugs) - Allow `--sandbox` to affect `ocamldep` invocations. Previously, they were wrongly marked as incompatible (ocaml/dune#6749, @rgrinberg) - Validate the command line arguments for `$ dune ocaml top-module`. This command requires one positional argument (ocaml/dune#6796, fixes ocaml/dune#6793, @rgrinberg) - Add a `dune cache size` command for displaying the size of the cache (ocaml/dune#6638, @Alizter) - Fix dependency cycle when installing files to the bin section with `glob_files` (ocaml/dune#6764, fixes ocaml/dune#6708, @gridbugs) - Handle "Too many links" errors when using Dune cache on Windows (ocaml/dune#6993, @nojb) - Allow the `cinaps` stanza to set a custom alias. By default, if the alias is not set then the cinaps actions will be attached to both `@cinaps` and `@runtest` (ocaml/dune#6991, @rgrinberg) - Add `(using ctypes 0.3)`. When used, paths in `(ctypes)` are interpreted relative to where the stanza is defined. (ocaml/dune#6883, fixes ocaml/dune#5325, @emillon) - Auto-detect `dune-workspace` files as `dune` files in Emacs (ocaml/dune#7061, @ilankri) - Add native support for polling mode on Windows (ocaml/dune#7010, @yams-yams, @nojb)

richardlford requested a review from christinerose as a code owner February 3, 2023 19:22

richardlford force-pushed the map-workspace-root-fix branch from a1ecd57 to f8cb8e2 Compare February 3, 2023 19:32

rgrinberg reviewed Feb 3, 2023

View reviewed changes

rgrinberg requested a review from snowleopard February 3, 2023 19:43

richardlford force-pushed the map-workspace-root-fix branch from f8cb8e2 to 5bd4b3d Compare February 3, 2023 19:55

snowleopard reviewed Feb 4, 2023

View reviewed changes

christinerose approved these changes Feb 6, 2023

View reviewed changes

doc/dune-files.rst Outdated Show resolved Hide resolved

doc/dune-files.rst Outdated Show resolved Hide resolved

richardlford force-pushed the map-workspace-root-fix branch from 805e862 to a59bc61 Compare February 6, 2023 18:51

richardlford force-pushed the map-workspace-root-fix branch from 68e0ebf to abe8660 Compare February 7, 2023 01:12

richardlford force-pushed the map-workspace-root-fix branch 2 times, most recently from bf1dc14 to 54416c6 Compare February 8, 2023 15:03

emillon added this to the 3.7.0 milestone Feb 8, 2023

emillon reviewed Feb 8, 2023

View reviewed changes

doc/dune-files.rst Show resolved Hide resolved

doc/dune-files.rst Outdated Show resolved Hide resolved

richardlford force-pushed the map-workspace-root-fix branch from 9646d71 to 326172a Compare February 8, 2023 17:59

Alizter reviewed Feb 8, 2023

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

richardlford force-pushed the map-workspace-root-fix branch 2 times, most recently from 80102e6 to badcb57 Compare February 8, 2023 21:24

richardlford force-pushed the map-workspace-root-fix branch from badcb57 to bf2569f Compare February 9, 2023 00:06

richardlford force-pushed the map-workspace-root-fix branch from bf2569f to 40e4591 Compare February 9, 2023 00:09

richardlford force-pushed the map-workspace-root-fix branch from 40e4591 to 5e43543 Compare February 9, 2023 11:21

emillon force-pushed the map-workspace-root-fix branch 2 times, most recently from 832bd3e to f3fa68c Compare February 9, 2023 14:57

emillon force-pushed the map-workspace-root-fix branch from f3fa68c to e70649b Compare February 9, 2023 15:00

Merge branch 'main' into map-workspace-root-fix

56fe9cd

emillon merged commit 7bdc8d6 into ocaml:main Feb 9, 2023

richardlford deleted the map-workspace-root-fix branch February 9, 2023 20:37

nojb mentioned this pull request Feb 20, 2023

Ocamldebug support broken in 3.6 #6929

Closed


		(map_workspace_root <bool>)

		Starting with Dune 3.0, Dune does this mapping by default. However,

Add dune-project stanza, map_workspace_root, to control workspace map… #6988

Add dune-project stanza, map_workspace_root, to control workspace map… #6988

Conversation

richardlford commented Feb 3, 2023 • edited Loading

rgrinberg Feb 3, 2023

Choose a reason for hiding this comment

richardlford Feb 3, 2023

Choose a reason for hiding this comment

rgrinberg commented Feb 3, 2023

richardlford commented Feb 3, 2023

rgrinberg commented Feb 3, 2023

richardlford commented Feb 3, 2023

rgrinberg commented Feb 4, 2023

snowleopard Feb 4, 2023

Choose a reason for hiding this comment

snowleopard Feb 4, 2023

Choose a reason for hiding this comment

richardlford Feb 6, 2023

Choose a reason for hiding this comment

richardlford Feb 6, 2023

Choose a reason for hiding this comment

snowleopard commented Feb 4, 2023

rgrinberg commented Feb 4, 2023

gasche commented Feb 4, 2023

christinerose left a comment

Choose a reason for hiding this comment

richardlford commented Feb 6, 2023 • edited Loading

Alizter commented Feb 6, 2023

Alizter commented Feb 6, 2023 • edited Loading

richardlford commented Feb 6, 2023

Alizter commented Feb 6, 2023 • edited Loading

richardlford commented Feb 6, 2023

richardlford commented Feb 7, 2023

richardlford commented Feb 8, 2023

emillon left a comment

Choose a reason for hiding this comment

richardlford commented Feb 8, 2023

Alizter commented Feb 8, 2023

richardlford commented Feb 8, 2023

Alizter commented Feb 8, 2023

richardlford commented Feb 8, 2023

richardlford commented Feb 8, 2023

richardlford commented Feb 8, 2023 • edited Loading

richardlford commented Feb 8, 2023

richardlford commented Feb 9, 2023

richardlford commented Feb 9, 2023

richardlford commented Feb 9, 2023

emillon commented Feb 9, 2023

richardlford commented Feb 3, 2023 •

edited

Loading

richardlford commented Feb 6, 2023 •

edited

Loading

Alizter commented Feb 6, 2023 •

edited

Loading

Alizter commented Feb 6, 2023 •

edited

Loading

richardlford commented Feb 8, 2023 •

edited

Loading