Remove incorrect validation step for dir targets

[gridbugs]

The validation in question was comparing directory targets discovered in the project from directories that are the target of enabled rules (rules which lack an enabled_if field, or whose enabled_if condition is false). This was incorrect as it doesn't account for directory targets which are the target of disabled rules so the validation fails if any rule with a directory target is disabled. The validation step was introduced in 1fd19d7 but there aren't any clues to why it was necessary.

Fixes #10310

[gridbugs]

@jeremiedimino tagging you since you wrote the original version of this validation step. Please let me know if I've misunderstood something.

[jeremiedimino]

Hi Stephen, This was a consistency check to ensure that an informed guess Dune made prior to reaching this point was correct. If you simply remove the check, it might lead to some soundness issues. If this code triggered an error, it’s likely that the code that makes the informed guess needs updating. It’s also possible that things have changed in the last two years and this check is no longer necessary. That I wouldn’t know as I stopped working on Dune a bit more than two years ago. I hope that helps clarify things. If that would be useful, I can go more in details of why we changed Dune to make this informed guess. I’m just a bit busy today, so I would have to do that another time.

…

On Fri, 29 Mar 2024 at 08:47, Stephen Sherratt ***@***.***> wrote: @jeremiedimino <https://github.com/jeremiedimino> tagging you since you wrote the original version of this validation step. Please let me know if I've misunderstood something. — Reply to this email directly, view it on GitHub <#10331 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A5J564IPNXRNAJWME7JNETDY2UTCBAVCNFSM6AAAAABFOEDOR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRWHA4TGMBTGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[emillon]

Hi Jeremie! 👋🏻

I agree, this check is here for a reason. Before removing it we would need a solid case. @rgrinberg will have the most up to date idea of the state of this.
What does the test do when the check is present? Can you submit a PR with just the test first?

[vouillon]

I suspect the correct fix is to check in function Dir_status.extract_directory_targets whether the rules are enabled.

[rgrinberg]

The validation step is indeed correct. Jerome has the right idea.

[gridbugs]

Thanks for the explanation @jeremiedimino! I will dig deeper.

[vouillon]

@gridbugs
I think you can add these lines to Dir_status.extract_directory_targets.

    | Rule { targets = Static { targets = l; _ }; enabled_if; loc = rule_loc; _ } ->
      let* available = (Expander.eval_blang expander enabled_if) in
      if not available then Memo.return acc else
      [...]

where expander is defined in Gen_rules.gen_rules_regular_directory:

  let* sctx = sctx in
  let* expander =
    let+ expander = Super_context.expander sctx ~dir in
    Dir_contents.add_sources_to_expander sctx expander
  in
  [...]

replicating what is done in Dir_contents.load_text_files and Simple_rules.user_rule.

[gridbugs]

Thanks for the tip @vouillon. I have a new PR based on your suggestion #10367.