Skip to content

fix: Allow anonymous actions to be directly attached to multiple aliases - #15581

Open
NatKarmios wants to merge 23 commits into
ocaml:mainfrom
NatKarmios:describe-rules-anon-actions
Open

fix: Allow anonymous actions to be directly attached to multiple aliases#15581
NatKarmios wants to merge 23 commits into
ocaml:mainfrom
NatKarmios:describe-rules-anon-actions

Conversation

@NatKarmios

@NatKarmios NatKarmios commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Change registered anonymous actions from an Anonymous_action.t Action_builder.t to a new Anonymous_action.Rule.t, which includes the loc, dir`, etc. (which was already computed ahead of time) and an ID (sharing ID space with regular rules).

Fixes #15576

Checklist

  • Tests added, if applicable.
  • Change log entry added for any user-facing changes.
  • Documentation added for any user-facing changes.

@NatKarmios
Handle anonymous actions in dune rules
f91cf4a 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Make rule locations optional
aace518 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Refactor alias to aliases
f3020d4 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Change printed location to filename:start_line format
3d1d6f9 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Move Anonymous_action_rule to Anonymous_action.Rule
47b30cc 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Memoize Build_system.execute_anonymous_action
758e8a4 
I'm not sure if this is actually necessary; in my testing the action
isn't rerun when multiple of its aliases are built.

Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Code review tweaks
b096764 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Tweak CLI flag name
5ea6006 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
NatKarmios force-pushed the describe-rules-anon-actions branch from e0ca6d8 to 5ea6006 Compare July 21, 2026 15:10
@NatKarmios
Add changelogs
96edb54 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Promote digest-sensitive tests
a0da334 
Signed-off-by: Nat Karmios <nat@karmios.com>
@Alizter
Alizter requested a review from rgrinberg July 22, 2026 09:00
Alizter
Alizter requested changes Jul 22, 2026
View reviewed changes

@Alizter Alizter left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't worry about the x86 macos job atm, that's unrelated. Could you split the dune rules change + reflection stuff into a separate (followup) PR? That will make reviewing this a bit easier.

The idea of attaching multiple aliases to a rule or anonymous action is interesting, but I will need to think about it's implications a bit more.

Alizter
Alizter reviewed Jul 22, 2026
View reviewed changes
Comment thread src/dune_engine/build_system.ml Outdated
Alizter
Alizter reviewed Jul 22, 2026
View reviewed changes
Comment thread src/dune_engine/build_system.ml Outdated
| Some a -> Alias.Name.to_string a ^ "-" ^ d
match act.aliases with
| [] -> d
| a :: _ -> Alias.Name.to_string a ^ "-" ^ d

@Alizter Alizter Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This means that the digest of the action is determined by the first alias?

@rgrinberg do we really want to be digesting the alias names at all here?

@Alizter Alizter Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose the question is how many times we expect:

(rule (alias a) (action (echo hi\n)))
(rule (alias b) (action (echo hi\n)))

when doing dune build @a @b.

With this change

(rule (alias a b) (action (echo hi\n)))
(rule (alias a c) (action (echo hi\n)))

dune build @b @c will only print hi once. But with

(rule (alias a c) (action (echo hi\n)))
(rule (alias b c) (action (echo hi\n)))

even dune build @c will print it twice.

@Alizter Alizter Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It does seem like a sensible decision to digest all the alias names then, but we obviously have to do so in some stable order.

@Alizter Alizter Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If the answer to my question on sharing actions with different aliases is that we want to share, then that would simplify the engine a bit since we wouldn't need to attach aliases to anon actions at all. It's not clear to me which one we want yet.

@NatKarmios NatKarmios Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, can't this basename tweak be removed? The digest itself is computed based on all the aliases (execute_action_generic).
I've tweaked this to alphabetically sort the alias names before adding to the digest in ba35683, would that be sufficiently stable?

@NatKarmios NatKarmios Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applying List.sort_uniq to the alias names before supplying them to the digest properly ensures that (aliases (a a)) doesn't run after (aliases (a)).
I think that the prepended basename isn't needed for normal operation, but a test (patch-back-source-tree.t) depends on it.
At the very least, I've made it select the (alphabetically) first alias name for prepending to ensure consistency.

Alizter
Alizter reviewed Jul 22, 2026
View reviewed changes
Comment thread src/dune_engine/build_system.ml Outdated
@NatKarmios

NatKarmios commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Could you split the dune rules change + reflection stuff into a separate (followup) PR? That will make reviewing this a bit easier.

Sure, done in 4d116a8

@NatKarmios
Revert "Memoize Build_system.execute_anonymous_action"
64f9c87 
This reverts commit 758e8a4.

Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Sort alias names when digesting
7af9b7a 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Revert Reflection / dune rules changes
16eec08 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
NatKarmios force-pushed the describe-rules-anon-actions branch from 4d116a8 to 16eec08 Compare July 22, 2026 13:49
@NatKarmios NatKarmios mentioned this pull request Jul 22, 2026
Draft
3 tasks
rgrinberg
rgrinberg reviewed Jul 22, 2026
View reviewed changes
Comment thread doc/changes/fixed/15581.md
Comment thread test/blackbox-tests/test-cases/alias/alias-multiple.t
@Alizter
Alizter self-requested a review July 22, 2026 14:35
Alizter
Alizter reviewed Jul 22, 2026
View reviewed changes
Comment thread src/dune_engine/rule.ml Outdated
@NatKarmios
Revert Rule.Anonymous_action.Rule.t
a07256e 
Signed-off-by: Nat Karmios <nat@karmios.com>
Alizter
Alizter reviewed Jul 22, 2026
View reviewed changes
Comment thread otherlibs/stdune/src/alias_name.mli Outdated
@NatKarmios
Oops, dead code
896bacc 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Prevent duplication by sorting aliases
001f487 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios NatKarmios changed the title fix(rules): Allow anonymous actions to be directly attached to multiple aliases fix: Allow anonymous actions to be directly attached to multiple aliases Jul 22, 2026
@NatKarmios NatKarmios mentioned this pull request Jul 22, 2026
Merged
rgrinberg pushed a commit that referenced this pull request Jul 24, 2026
@NatKarmios
test: Surprising behaviour on anonymous actions with multiple aliases (…
1bdc60d 
…#15590)

Related to #15576 & #15581 

Adds 3 tests for anonymous actions with multiple aliases, specifically:
- Building one of an action's aliases can pull in another alias
- Duplicating or re-ordering aliases in a rule does not cause a re-run

Note that all these tests currently fail, but are fixed by #15581:
<details>
<summary><b>Test results</b></summary>

```diff
File "test/blackbox-tests/test-cases/alias/alias-multiple.t", line 1, characters 0-0:
------ test/blackbox-tests/test-cases/alias/alias-multiple.t
++++++ test/blackbox-tests/test-cases/alias/alias-multiple.t.corrected
File "test/blackbox-tests/test-cases/alias/alias-multiple.t", line 139, characters 0-1:
 |
 |A rule attached to several aliases must not make one alias pull in unrelated
 |contributions to another alias. Here alias [a] also receives an unrelated
 |action; building [b] must run only the shared action.
 |  $ cat > dune << EOF
 |  > (rule
 |  >  (aliases a b)
 |  >  (action (echo "I have run\n")))
 |  > (rule
 |  >  (alias a)
 |  >  (action (echo "unrelated\n")))
 |  > EOF
 |
 |  $ dune clean
 |  $ dune build @b
 |  I have run
+|  unrelated
 |
 |Building [a] still runs both the shared action and the action attached only
 |to [a].
 |  $ dune clean
 |  $ dune build @A
 |  I have run
 |  unrelated
 |
 |The set of aliases determines the action's identity: neither duplicating an
 |alias nor reordering the aliases should re-run the action.
 |
 |Duplicating an alias does not re-run the action:
 |  $ cat > dune << EOF
 |  > (rule
 |  >  (aliases a)
 |  >  (action (echo "I have run\n")))
 |  > EOF
 |  $ dune clean
 |  $ dune build @A
 |  I have run
 |  $ cat > dune << EOF
 |  > (rule
 |  >  (aliases a a)
 |  >  (action (echo "I have run\n")))
 |  > EOF
 |  $ dune build @A
+|  Error: Dependency cycle between:
+|     alias a in dune:1
+|  [1]
 |
 |Reordering the aliases does not re-run the action:
 |  $ cat > dune << EOF
 |  > (rule
 |  >  (aliases a b)
 |  >  (action (echo "I have run\n")))
 |  > EOF
 |  $ dune clean
 |  $ dune build @A @b
 |  I have run
 |  $ cat > dune << EOF
 |  > (rule
 |  >  (aliases b a)
 |  >  (action (echo "I have run\n")))
 |  > EOF
 |  $ dune build @A @b
+|  I have run
```

</details>

---------

Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios NatKarmios closed this Jul 24, 2026
@NatKarmios
NatKarmios deleted the describe-rules-anon-actions branch July 24, 2026 15:12
@NatKarmios
NatKarmios restored the describe-rules-anon-actions branch July 24, 2026 15:13
@NatKarmios NatKarmios reopened this Jul 24, 2026
@Alizter
Alizter self-requested a review July 24, 2026 15:14
@NatKarmios

NatKarmios commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

(Sorry, didn't mean to close/reopen)
I've rebased and updated the tests.

rgrinberg
rgrinberg requested changes Jul 26, 2026
View reviewed changes

@rgrinberg rgrinberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's just get rid of the aliases from the definition of anonymous actions entirely. I can't think of a good reason to have them.

@NatKarmios
Change aliases field of Rule.Anonymous_action.t to `attached_to_a…
0a9603b 
…lias : bool`

Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios

NatKarmios commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Let's just get rid of the aliases from the definition of anonymous actions entirely. I can't think of a good reason to have them.

Done; I've replaced the aliases field with attached_to_alias : bool.
I think we still need the distinction because of --force.

@Alizter

Alizter commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Done; I've replaced the aliases field with attached_to_alias : bool. I think we still need the distinction because of --force.

Which behaviour of --force would removing aliases entirely be breaking?

@NatKarmios

NatKarmios commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Which behaviour of --force would removing aliases entirely be breaking?

Currently, --force only applies to anonymous actions attached to aliases. There are various internal actions that are not attached to aliases (e.g. ocamldep rules), which I presume we wouldn't want to rerun on --force.

@NatKarmios

NatKarmios commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Let's just get rid of the aliases from the definition of anonymous actions entirely. I can't think of a good reason to have them.

Ah, there's one reason; Dpath.analyse_target takes the alias name used on the stamp file to help with debugging information (assigning the anon. action's "purpose" to the alias); this is why a bunch of tests are broken now, all these actions are just being displayed as (internal).

@Alizter

Alizter commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

I think --force was already a bit arbitrary, so we shouldn't worry about keeping it's behaviour the same. If we think --force will be useful, which is already a bit dubious, it would be better to spend time giving it a better meaning at some later stage.

Having an anon action print differently depending on if it has an alias or not also seems pretty arbitrary. I don't think we are losing information by making these (internal). The trace file via dune trace cat | jq contains all the important information and --display=short was introduced only to appease some make users.

@NatKarmios
Remove attached_to_alias
5085f8f 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
Change anonymous action short-display from "(internal)" to "(anonymous)"
3d2d3cd 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
tests: handle changed alias display
ab93271 
Signed-off-by: Nat Karmios <nat@karmios.com>
@NatKarmios
NatKarmios force-pushed the describe-rules-anon-actions branch from 2bab5ea to ab93271 Compare July 28, 2026 13:35
@NatKarmios

NatKarmios commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've tweaked anonymous actions in short-display from "(internal)" to "(anonymous)" (the former is also used for processes with Internal_job metadata, so I thought this is a smidge less ambiguous).

I've also changed the tests to adapt to this; in some situations I've promoted the change from printing the alias to (anonymous), and in others I've tried to adapt the test to $ dune trace cat | jq_dune.

The anon-action digest changes have also introduced a couple of subtle cases where identical actions under different aliases are expected to re-run, but don't; I've inserted $ dune clean in these tests where needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrinberg rgrinberg rgrinberg requested changes
@Alizter Alizter Awaiting requested review from Alizter

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

User rules with multiple aliases cause surprising inter-alias dependencies

3 participants

@NatKarmios @Alizter @rgrinberg