output relative paths in error output to improve reproducibility

[avsm]

In RWO, we have expect tests that fail, but result in host-paths being leaked into the expect tests. For example

$ dune runtest
       patdiff (internal) (exit 1)
  (cd _build/default && /home/yminsky/Documents/code/rwo/_build/install/default/bin/patdiff -keep-whitespace -location-style omake -ascii test.ml test.ml.corrected)

could be

$ dune runtest
       patdiff (internal) (exit 1)
  (cd _build/default && ../install/default/bin/patdiff -keep-whitespace -location-style omake -ascii test.ml test.ml.corrected)

in order to have the same output irrespective of what host its run on. See realworldocaml/book#3448 for more.

[avsm]

We're aiming to deliver a RWOv2 to our publisher in early September, so if this could be snuck into a dune point release that would be very helpful to us getting the book stable in August ;-)

[rgrinberg]

I checked dune's code and we already make an attempt to make paths from _build relative. Do you have a special setup in rwo where you invoke dune from inside dune?

[voodoos]

@ejgallego should this be added to the 2.9.1 milestone ?

[avsm]

That's exactly what seems to be happening @rgrinberg -- we're calling dune-in-dune in order to evaluate examples within the book. It's calling dune with a --root within the tree. But why would that make the invoked dune output an absolute build path?

[nojb]

But why would that make the invoked dune output an absolute build path?

If memory serves the logic only outputs relative paths when they are children of the _build/<context> directory. Is that the case here?

[yminsky]

I'm not sure. This PR shows where the bad paths show up:

realworldocaml/book#3447

And here's the error:

https://ci.ocamllabs.io/github/realworldocaml/book/commit/a4fea553f95ee4946e9b59ab5543f24102c4f06d/variant/debian-10-4.10

Scroll to the bottom to see the problematic corrections.

The paths do look like they're under _build. We're doing something a little weird here, where we want to run dune-within-dune, because we're invoking dune as part of an example, as part of the overall build of the book.

[rgrinberg]

But why would that make the invoked dune output an absolute build path?

It happens because the outer instance of dune is appending its own build directory to PATH. The inner dune observes PATH and treats all binaries originating from it as absolute paths. I don't see how we can fix this problem in dune itself without adding special handling for running dune inside dune.

However, some possible workarounds are:

• post process the output to rewrite the paths from the outer dune.
• set the build directory in the outer dune to something that is reproducible e.g. dune build --build-dir=/tmp/rwo.
• include a build a in the inner dune that will copy the patdiff binary from the outer dune.

[ejgallego]

@ejgallego should this be added to the 2.9.1 milestone ?

I would have no problem if we converge on a solution.

[avsm]

I can't quite find a clean workaround for this in RWO. The specific stanza we had is this:

```sh dir=examples/erroneous/echo_test_original/test,unset-INSIDE_DUNE
  $ dune runtest
  Entering directory '/home/yminsky/Documents/code/rwo/_build/default/book/testing/examples/erroneous/echo_test_original'
       patdiff (internal) (exit 1)
  (cd _build/default && /home/yminsky/Documents/code/rwo/_build/install/default/bin/patdiff -keep-whitespace -location-style omake -ascii test/test.ml test/test.ml.corrected)
  ------ test/test.ml
  ++++++ test/test.ml.corrected
  File "test/test.ml", line 11, characters 0-1:
   |open! Core
   |open! Async
   |open Helpers
   |

What's going on here is that there is a README.md file with an mdx stanza, and the environment entirely built using a duniverse (the "outer dune"). The patdiff binary is grabbed from the outer dune and so this is leaked into the mdx evaluation (which deliberately unsets INSIDE_DUNE, as the intention is to show the results of running dune in the book text).

One other workaround: is it necessary to show the results of a patdiff exit 1 in the build output? This is the normal course of events if there is a diff, so perhaps we could just show the diff and not the exact arguments to patdiff (unless in verbose mode)?

I.e. instead of:

  $ dune runtest
  Entering directory '/home/yminsky/Documents/code/rwo/_build/default/book/testing/examples/erroneous/echo_test_original'
       patdiff (internal) (exit 1)
  (cd _build/default && /home/yminsky/Documents/code/rwo/_build/install/default/bin/patdiff -keep-whitespace -location-style omake -ascii test/test.ml test/test.ml.corrected)
  ------ test/test.ml
  ++++++ test/test.ml.corrected
  File "test/test.ml", line 11, characters 0-1:
   |open! Core
   |open! Async
   |open Helpers
   |
   |let%expect_test "test uppercase echo" =

just show

  $ dune runtest
  ------ test/test.ml
  ++++++ test/test.ml.corrected
  File "test/test.ml", line 11, characters 0-1:
   |open! Core
   |open! Async
   |open Helpers
   |
   |let%expect_test "test uppercase echo" =

for the diff.

[avsm]

Another alternative is also to be able to manually specify a patdiff binary. It's hardcoded right now, but an env variable to (e.g.) call DUNE_DIFF=rwo-patdiff would let us manually move the binary to somewhere friendly on the PATH that is relative.

[rgrinberg]

One other workaround: is it necessary to show the results of a patdiff exit 1 in the build output? This is the normal course of events if there is a diff, so perhaps we could just show the diff and not the exact arguments to patdiff (unless in verbose mode)?

This is a good idea. There's already a hack to clean up the output of ocamlc. We could do something similar for diff and patdiff. Note that to be consistent with our hack for ocamlc, this will only be enabled in the --dev mode.

Another alternative is also to be able to manually specify a patdiff binary. It's hardcoded right now, but an env variable to (e.g.) call DUNE_DIFF=rwo-patdiff would let us manually move the binary to somewhere friendly on the PATH that is relative.

One of the workarounds I've mentioned to is to include the following stanza somewhere in your dune projects:

(env
  (_
   (binaries ./patdiff.exe)))
(copy /path/to/patdiff ./patdiff.exe)

Your suggestion would replace the env above with an environment variable. But you'd still need that copy stanza to accomplish moving the binary where it would be treated as relative (i.e. inside the workspace). So you don't gain much by this as you still need to write code in a dune file.