fix: keep menhir .conflicts files (#6865)

[Tchou]

Add the .conflicts file to targets of menhir stanzas when the --explain flag is present and menhir is not called with --only-tokens.

[Tchou]

I don't understand. I added a test demonstrating that it does work in env. Or maybe the test is not complex enough ? like there should be several distinct env ?
The test is in explain-env.t with two sub-directories and two parsers where --explain works for both.

Note at this moment I can only test with env in a dune file. When I try to mention menhir_flags in dune-workspace, dune crashes (I opened an issue for that).

Or else I don't understand what not working means (which may very well be the case).

[Alizter]

@Tchou I apologise, I read a little fast earlier. Let me take a closer look. You can disregard my comment from earlier.

[Alizter]

Could you do dune build _build/default/a/parser.conflicts directly so that we can see Dune recognizes it as a target and isn't producing it by accident. You can also do 2> /dev/null to remove the output and remove --debug-dependency-path so that the test is a little easier to read.

[Alizter]

The reason I ask is that if Dune isn't actually aware of the .conflicts file in this situation then it cannot be depended on, or expected to persist between builds.

[Tchou]

New attempt (the diff with main is small so I squashed and force pushed since the solution I ended up with handle both cases).

I think this works well but the intricacies and differences of Memo.t vs Action_builder.t are lost on me. I added a few more test cases to also handle .cmly files. With this commit, everything should work as intended:

• .cmly and .conflicts files are preserved in _build
• they are correctly recognized as targets (dune build _build/default/foo.{conflicts,cmly} is happy)
• it works whether the flags are passed through the menhir stanza or through env.

[Alizter]

@Tchou What's left to undraft this?

[Tchou]

Hi,
sorry it slipped by. So it's not satisfactory at the moment since I don't understand how things could work in all generality. So long story short:

• if --explain files are passed in env or in the dune file directly all is well (with current patch)
• BUT if the flags are given with an :include file that fails (dune actually report some internal error due to me doing crazy stuff). And this breaks menhir CI too since in the test cases of the menhir tools, include files are used to test menhir with various flags (what I mean is that in my branch, will musing around trying to understand how I could test the content
  of :include files, i must have broken it, I can of course backtrack but it feels silly to not handle this case).

I have gone through dune sources and I cannot understand how to do get the expanded include file, from within the menhir plugin. Ideally it should go like this:

• wait until the include file is expanded as actual flags
• at that moment parse the flags and add new targets (the .conflict file) like it is done now.
  My limited understanding is that an :included file is also a dependency of the rule (so that the rule is recomputed if the file changes) but then expansion seem to occur pretty late.

I couldn't understand dune's architecture well enough to know whether this is possible.
If there is some documentation or examples of other plugins/rules doing it I could try to look into it again.

[nojb]

Ideally it should go like this:

• wait until the include file is expanded as actual flags
• at that moment parse the flags and add new targets (the .conflict file) like it is done now.
  My limited understanding is that an :included file is also a dependency of the rule (so that the rule is recomputed if the file changes) but then expansion seem to occur pretty late.

I couldn't understand dune's architecture well enough to know whether this is possible.

Hello Kim! @rgrinberg or @snowleopard will correct me, but I understand that at the moment targets must be statically known by Dune, so it is not possible to have the set of targets depend on things like :include which need Dune to build stuff.

[rgrinberg]

Replaced by #9512