Handle non-existing file in print_diff

[bobot]

Diff command don't like that one of the file doesn't exist. The solution implemented is for dune to print the diff when one of the files doesn't exists.

However the current way we do diffing seems strange to me:

• A Format.eprintf "%a@?" Loc.render (Loc.pp loc); is used in the middle of the code, so the line is printed at random place
• The output of the diff command is directly connected to the stdout, so not buffered.

How to fix that (if it is needed)?

• Perhaps we should do like for other commands (run ...), but should the output of (diff ...) be redirected in case of (write-stdout-to ...)?

So it is still a draft:

• It circumvents also user diff in this case, is it a problem?
• It uses Printf.printf, it should be a problem.
• It always uses ANSI colors, it is a problem

[ghost]

Just to clarify, this is so that we don't have to do touch x the first time, right?

[bobot]

Yes it is a follow-up of #3795. I though the tests were enough but since inside dune diffing is not done, the problem was not apparent.

[ghost]

Ack. For the Format.eprintf, we could rely on Action_exec rather than calling Format.eprintf followed by Process.run. For the code added in this PR, why not construct an error message rather than using Printf.printf?

[bobot]

I just understood that Process.run was doing the delay of the output itself, that's why it is not mangled, even if it goes to stdout directly.

For the Format.eprintf, we could rely on Action_exec

Can you precise your idea? Should we print the message before going in print_diff in action_exec? I think for the user the output of this Format.eprintf should go just before the diff output, so everything should be buffered together. For example print_diff could be given the eenv.std*_to of Action_exec. That would make th output of (diff..) similar to (run ...) but I'm not sure it is problematic.

For the code added in this PR, I would like to print it in a way similar to the one used by the diff tools, so that it is not too dissimilar. So error message could be a solution, but I will see once I understand what must be done in the normal case.

[ghost]

The idea was to buffer them together indeed. But thinking about it again that's not going to help.

Process.run always buffer the output to the terminal, and when the process fails, the output is part of the exception raised and is then printed by Report_error. Now that I say that, it seems that the output in the normal case could get mangled as well. In fact, the location printing could even get mangled with the Done: x/y (jobs: z) progress line. We should at least use Console.print to avoid the latter.

For the code added in this PR, I would like to print it in a way similar to the one used by the diff tools, so that it is not too dissimilar. So error message could be a solution, but I will see once I understand what must be done in the normal case.

Seems reasonable. In the normal case, we print the location in the same format as the compiler, so that emacs can pick it up. We should probably do the same when one of the files is absent. This way the user can quickly jump to it and do M-x dune-promote.

BTW, I was checking the various diff command and I found that:

• patdiff handles the case when one of the file is absent:

  $ rm -f x
  $ echo toto > y
  $ patdiff x y
  ------ /dev/null
  ++++++ y
  @|-1,0 +1,1 ============================================================
  +|toto
  $ patdiff y x
  ------ y
  ++++++ /dev/null
  @|-1,1 +1,0 ============================================================
  -|toto
  

• diff has a -N option to treat absent files as empty
• git diff doesn't have such an option and choke on missing files. Though when using it in a git repo and a new file is added, it uses /dev/null for the missing file

This makes me think that we could explicitly use /dev/null for absent files. WDYT?

[bobot]

This makes me think that we could explicitly use /dev/null for absent files. WDYT?

When we do that the diff tool print /dev/null instead of the non-existing file name. I though it was not very informative.

[ghost]

I guess we don't often see the mangling happening because Dune prefers patdiff and patdiff does the location printing itself. But I expect it happens when patdiff is not installed.

[ghost]

When we do that the diff tool print /dev/null instead of the non-existing file name. I though it was not very informative.

That's true, but since we print the location before the diff the user knows what file we are talking about.

[bobot]

I'm seeing the mangling, because my users don't use by default patdiff (so I removed it).

[bobot]

We print the location of only one file, not both.

[ghost]

Here is a simple way to avoid the mangling: we could pass the location to Process.run so that it adds it to the exception.

[ghost]

We print the location of only one file, not both.

Yes, but that seems fine to me. Only the first file is promoted to the source tree. The second one is the correction. The second file being missing is not a very interesting case. In fact, I'm not even sure we can reach the case where the second file is missing in print_diff.ml. More precisely:

• either the diff is optional and if the second file is missing we do nothing
• either the diff is not optional and if the second file is missing we fail before that (in Diff.eq_files)

[craigfe]

I ran what I believe is the same problem when using a set of diff-promote rules like this. For those interested, the symptom was a failure from Dune about resolving symlinks:

❭ dune runtest
Error: Unable to resolve symlink
_build/default/test/ppx_repr/deriver/passing/recursive.expected

Running dune promote does indeed fix the issue, though I didn't think of doing that until I came here. Prior to #3795, I get the following more descriptive error:

❭ dune runtest
File "test/ppx_repr/deriver/passing/dune.inc", line 436, characters 0-98:
436 | (rule
437 |  (alias runtest)
438 |  (package ppx_repr)
439 |  (action
440 |   (diff recursive.expected recursive.actual)))
Error: No rule found for test/ppx_repr/deriver/passing/recursive.expected

[ghost]

FTR, we had a discussion about this with @bobot and end up settling for the following design:

• for the printing interleaving problem, we will simply pass the location to Process.run which will then put it in the exception
• for the case where one file is missing, we will use /dev/null instead and pass that to the diff command. This avoids having to mimic the output of the diff command, which is dependant on the diffing tool being used

(@bobot does match what your memory of this discussion?)

I don't think it quite solves your problem @craigfe though. How did you ended up with a symlink there BTW? FTR, a good tool to debug such errors is to pass --debug-dependency-path. Then Dune will show what path lead it to this error.

[rgrinberg]

@bobot do you plan to work on this for 3.0?

[rgrinberg]

Please re-open if this is still relevant