fix(engine): directory targets sanity check#5754
Conversation
|
The new check seems too weak to me. A better check should clearly depend on whether the targets are actually redirected or not. Also, I don't like that we can't test it. I'd rather we don't support directory targets when they are redirected at all instead of weakening the current check. |
Yes, I agree. Let me improve the check.
This PR came out of #5695. Where we used directory targets to generate documentation rules for coq. We can easily show that the new check fixes the issue there. It's not ideal of course, and I do think that we should unit test the public api of dune_engine as well. Alas, we're not ready for that yet.
That would be a shame because the feature is very convenient for doc targets. |
d5129fa to
858672c
Compare
|
@snowleopard I put a new version of the sanity check. It appears to be correct in all cases and the validation error is raised as soon as possible. As a bonus, I improved the error message to guide the developer a little better. Previously, the error left me wondering what was wrong, the new one is easier to understand imo |
858672c to
29b3fbb
Compare
src/dune_engine/load_rules.ml
Outdated
| not | ||
| (Path.Build.Map.equal real_directory_targets directory_targets | ||
| ~equal:(fun _ _ -> | ||
| (* the locations don't matter and aren't expected to match *) |
There was a problem hiding this comment.
Do locations actually not match? Rules.directory_targets seems to provide correct locations, so if they don't match, then it's directory_targets that is to blame.
If locations really don't matter, why require to provide locations in directory_targets in the first place?
If they do matter, why allow the mismatch?
There was a problem hiding this comment.
I think my comment just needs to be clarified. One is a location for the declaration of the directory target, while the other is a location for the rule that generated the target.
If possible, it would be nice if they matched, but sometimes we might not know the rule's location in advance.
There was a problem hiding this comment.
Oh, I see. Yes, I think this needs clarifying. Not only in this comment, but also in the comments in Rules.directory_targets and Gen_rules?
snowleopard
left a comment
There was a problem hiding this comment.
Thanks, looks good now (modulo the Loc.t confusion).
I'm a bit puzzled though: it's not obvious that this change actually changes behaviour. I suppose Rules.directory_targets somehow manages to be more accurate than rules_here.by_directory_targets. Why is that?
|
rules_here.by_directory_targets is the directory targets for the directory where we are loading the rules. While Rules.directory_targets is for all produced rules which includes subdirectories. The current check fails because it compares a declaration for all produced rules vs the directory targets in the dir we are loading. My first attempt at the fix was to narrow the declaration to only the subset that matters to the dir we are loading. This new fix checks the full declaration against the full set of directory targets from the produced rules. |
|
Got it, thanks! |
The current sanity check for directory targets does not take into account rule redirection. This commit fixes the check to account for all generated subdirectories and improves the error message. Signed-off-by: Rudi Grinberg <me@rgrinberg.com> ps-id: E2A4355D-89E1-4A92-92F7-918981AAB107
29b3fbb to
a761c81
Compare
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.3.0) CHANGES: - Sandbox preprocessing, lint, and dialect rules by default. All these rules now require precise dependency specifications (ocaml/dune#5807, @rgrinberg) - Allow list expansion in the `pps` specification for preprocessing (ocaml/dune#5820, @Firobe) - Add warnings 67-69 to dune's default set of warnings. These are warnings of the form "unused X.." (ocaml/dune#5844, @rgrinbreg) - Introduce project "composition" for coq theories. Coq theories in separate projects can now refer to each other when in the same workspace (ocaml/dune#5784, @Alitzer, @rgrinberg) - Fix hint message for ``data_only_dirs`` that wrongly mentions the unknown constructor ``data_only`` (ocaml/dune#5803, @lambdaxdotx) - Fix creating sandbox directory trees by getting rid of buggy memoization (@5794, @rgrinberg, @snowleopard) - Handle directory dependencies in sandboxed rules. Previously, the parents of these directory dependencies weren't created. (ocaml/dune#5754, @rgrinberg) - Set the exit code to 130 when dune is terminated with a signal (ocaml/dune#5769, fixes ocaml/dune#5757) - Support new locations of unix, str, dynlink in OCaml >= 5.0 (ocaml/dune#5582, @dra27) - The ``coq.theory`` stanza now produces rules for running ``coqdoc``. Given a theory named ``mytheory``, the directory targets ``mytheory.html/`` and ``mytheory.tex/`` or additionally the aliases `@doc` and `@doc-latex` will build the HTML and LaTeX documentation repsectively. (ocaml/dune#5695, fixes ocaml/dune#3760, @Alizter) - Coq theories marked as `(boot)` cannot depend on other theories (ocaml/dune#5867, @ejgallego) - Ignore `bigarray` in `(libraries)` with OCaml >= 5.0. (ocaml/dune#5526, fixes ocaml/dune#5494, @moyodiallo) - Start with :standard when building the ctypes generated foreign stubs so that we include important compiler flags, such as -fPIC (ocaml/dune#5816, fixes ocaml/dune#5809).
The current check is wrong because it assumes that all directory targets
are going to be produced for the current directory. Some directories
forward their rules to parents, which means that sub-directories should
only bother checking for targets that are declared for them.
It's not possible to reproduce this test using rules defined in
dunefiles at the moment. It's only possible to reproduce this by defining rules using theDune_engineapi.Another option is to skip this check altogether when invoking the rules through a sub-dir (a bit trickier to do).