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.

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

test: aliases field of user rules generates multiple actions #11557

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Mar 22, 2025

In a similar vain to #11547 user defined rules were generating multiple actions which meant building both aliases at the same time was running the action twice. We fix this issue by making the other aliases rely on the first alias in the list. (This might be fragile lets see).

Memo.parallel_iter aliases ~f:(fun alias ->
let alias = Alias.make ~dir alias in
Alias_rules.add sctx ~alias ~loc:rule.loc action)
match List.map ~f:(Alias.make ~dir) aliases with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we have the same issue above with Aliases_with_targets ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There we are using Rules.Produce.Alias.add_deps which correctly registers the deps without introducing any anonymous actions. Here we are using Super_context.add_alias_action which is creating an anonymous action. This is a wrapper around Rules.Produce.Alias.add_action which for our purposes is probably not what we want.

I've gone ahead and replaced this with Rules.Produce.Alias.add_deps here as well and added a test to make sure the with_targets version is treated the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can confirm that with_targets did not have the issue I am fixing here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you look at my latest push, there is a test for both with targets and without and as I mentioned the one with targets does not run twice. The bug I am fixing here only appears to be an issue for rules with no targets.

@Alizter Alizter force-pushed the user-aliases-multi-run branch from 023bc93 to 8378784 Compare March 23, 2025 12:11
@Alizter Alizter marked this pull request as ready for review March 24, 2025 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants