Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(rules): merge allowed subdirs correctly for Redirect_to_parent rules #7207

Merged
merged 4 commits into from
Mar 16, 2023
Merged

fix(rules): merge allowed subdirs correctly for Redirect_to_parent rules #7207

merged 4 commits into from
Mar 16, 2023

Conversation

anmonteiro
Copy link
Collaborator

@anmonteiro anmonteiro commented Mar 2, 2023 •
edited
Loading

  • After feature: Redirect_to_parent improved #6637, the engine may redirect rule loading to the parent if it finds that it needs to find rules in the parent directory
  • Part of generating rules in a directory is also finding out which sub-directories Dune is allowed to traverse; we store that in the record fields build_dir_only_sub_dirs
  • However, while Build_config.Rules.t can now contain rules for multiple directories, build_dir_only_sub_dirs is still a flat set of sub-directories, leading Dune to (wrongly) assume that both directories which had their rules combined can traverse the same set of sub-directories, when that's not the case.
  • This diff fixes that inconsistency by tracking the allowed set of sub-directories at each level
    • Specifically, this change unlocks traversing all directories when finding rules for recursive aliases, not just the directories in the source tree.

@anmonteiro anmonteiro requested a review from rgrinberg March 2, 2023 01:24
@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch 2 times, most recently from 663ede5 to 1ffaebc Compare March 2, 2023 09:46
@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch 2 times, most recently from a9841e2 to eafb7fd Compare March 2, 2023 17:29
@anmonteiro
Copy link
Collaborator Author

Moved the automatic subdir stuff over to #7208 and fixed your review comments.

@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch from eafb7fd to 9b797ce Compare March 2, 2023 19:44
@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch 3 times, most recently from a1257c2 to c4fa88e Compare March 8, 2023 20:06
@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch from c4fa88e to 731986a Compare March 13, 2023 22:26
@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch 2 times, most recently from 8fd380b to d9105b3 Compare March 14, 2023 01:33
@anmonteiro anmonteiro requested a review from snowleopard March 15, 2023 01:29
@anmonteiro
Copy link
Collaborator Author

@snowleopard I requested your review on this one since it touches the engine and you were a reviewer of #6637.

@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch from 522eaea to 1f35f58 Compare March 15, 2023 08:11
@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch from 1f35f58 to 09eba42 Compare March 15, 2023 08:21
@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch from e251d39 to da8e529 Compare March 15, 2023 09:51
snowleopard
snowleopard approved these changes Mar 15, 2023
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing all my comments. Looks good now!

@anmonteiro
Copy link
Collaborator Author

Thanks!

@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch 2 times, most recently from 907b19a to cdaacbd Compare March 15, 2023 22:29
@anmonteiro
Copy link
Collaborator Author

Squashed my commits.

@anmonteiro anmonteiro mentioned this pull request Mar 16, 2023
Merged
rgrinberg
rgrinberg reviewed Mar 16, 2023
View reviewed changes
src/dune_engine/load_rules.ml
@@ -501,14 +501,15 @@ end = struct

module Normal = struct
type t =
{ build_dir_only_sub_dirs : Subdir_set.t
{ build_dir_only_sub_dirs : Build_config.Rules.Build_only_sub_dirs.t
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In make_rules_gen_result we verify that both rules and directory_targets only contain valid keys. We should similar validation for this map as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did that in 72bea81

rgrinberg
rgrinberg reviewed Mar 16, 2023
View reviewed changes
src/dune_rules/gen_rules.ml Outdated
@@ -405,29 +413,34 @@ let rec under_melange_emit_target ~dir =
| None -> under_melange_emit_target ~dir:parent
| Some stanza -> Memo.return @@ Some { stanza_dir = parent; stanza }))

let melange_emit_rules sctx { stanza_dir; stanza } =
let rules_for ~dir components rules =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the way this is factored is making less clear. In some functions, we pass components, elsewhere, we use this components to compute the automatic_subdirs_map. How about we just compute automatic_subdirs_map right away and pass that around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check 33a7336 where I implemented your suggestion.

@anmonteiro anmonteiro force-pushed the anmonteiro/fix-redirect-to-parent branch from 04e678c to 33a7336 Compare March 16, 2023 03:58
@rgrinberg rgrinberg merged commit 20ad5e1 into ocaml:main Mar 16, 2023
@anmonteiro anmonteiro mentioned this pull request Mar 16, 2023
Merged
@anmonteiro anmonteiro deleted the anmonteiro/fix-redirect-to-parent branch March 16, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

@snowleopard snowleopard snowleopard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anmonteiro @rgrinberg @snowleopard