Skip to content

Fix rpc not transferring promotion warnings to the client #12604

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
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix rpc not transferring promotion warnings to the client #12604

wants to merge 7 commits into from

Conversation

@ElectreAAS
Copy link
Collaborator

@ElectreAAS ElectreAAS commented Oct 20, 2025 •
edited
Loading

Fixes #12578, and only looks like it fixes #12577, but doesn't, it was a red herring. See discussion on latter PR.

What was missing

  • A proper way for RPC requests to return warnings encountered during build.

Alizter
Alizter reviewed Oct 21, 2025
View reviewed changes
rgrinberg
rgrinberg requested changes Oct 22, 2025
View reviewed changes
ElectreAAS
ElectreAAS commented Oct 29, 2025
View reviewed changes
ElectreAAS
ElectreAAS commented Oct 29, 2025
View reviewed changes
src/promote/diff_promotion.ml
|> List.concat_map ~f:(fun (dst, srcs) ->
List.map srcs ~f:(fun (src, staging) -> { File.src; staging; dst }))
in
let sorted_missing = List.rev missing in
Copy link
Collaborator Author

@ElectreAAS ElectreAAS Oct 29, 2025

Choose a reason for hiding this comment

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

group_by_targets will sort all files, but the fold above reverses the list of missing files.
The absence of this reverse is what had lead to a swap in test-cases/promote/old-tests.t that was present in this PR before I noticed

@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch from 7346050 to 0d45f52 Compare October 29, 2025 10:27
@ElectreAAS ElectreAAS requested a review from rgrinberg October 29, 2025 16:20
ElectreAAS
ElectreAAS commented Oct 31, 2025
View reviewed changes
@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch 2 times, most recently from 2b5ad52 to c6e4024 Compare November 5, 2025 15:53
@ElectreAAS
Copy link
Collaborator Author

ElectreAAS commented Nov 5, 2025

I just rebased on main.
I believe I addressed your request @rgrinberg, and this is ready for a second pass

rgrinberg
rgrinberg approved these changes Nov 5, 2025
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Haven't looked too closely, but everything seems sensible at first glance.

shonfeder
shonfeder reviewed Nov 5, 2025
View reviewed changes
src/promote/diff_promotion.ml
dump_db db
;;

(* Returns the list of files that were in [files_to_promote]
Copy link
Member

@shonfeder shonfeder Nov 5, 2025

Choose a reason for hiding this comment

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

Thanks for adding the comments!

@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch 2 times, most recently from 69ba843 to 3ad52a5 Compare November 7, 2025 16:15
@shonfeder shonfeder requested a review from Alizter November 10, 2025 17:38
Alizter
Alizter reviewed Nov 10, 2025
View reviewed changes
@Alizter
Copy link
Collaborator

Alizter commented Nov 11, 2025

I think it would be better to split the two changes here. I'm fairly certain the promotion database clearing bug has been solved here, so I think that can be reviewed and merged quickly. The warning forwarding however has some potential issues and I think need a more careful review.

@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch 2 times, most recently from 6c4c3b8 to 2bd4cfe Compare November 14, 2025 16:57
@ElectreAAS
Copy link
Collaborator Author

ElectreAAS commented Nov 14, 2025

I just did a pass, moving the warning state from a variant in Build_outcome_with_diagnostics.t to a field in Compound_user_error.t according to discussions we've had in passing.
During that pass, i tried to only keep the warning changes and to not fix #12577, but according to the tests, the fix is still there, so I'm puzzled.

I tested locally and it doesn't seem to be the fold problem mentionned above (#12604 (comment))...

@Alizter
Copy link
Collaborator

Alizter commented Nov 14, 2025

@ElectreAAS in that case it sounds like there is some racy behaviour. Could you intersperse some sleep commands to make sure things are finishing before you go onto the next command? Recall that dune build connecting to a watch server may exit before the server is done doing stuff to the promotion database.

@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch from 2bd4cfe to b9e2d95 Compare November 14, 2025 17:27
rgrinberg
rgrinberg reviewed Nov 15, 2025
View reviewed changes
rgrinberg
rgrinberg reviewed Nov 15, 2025
View reviewed changes
@ElectreAAS ElectreAAS changed the title Fix rpc promotion being too optimistic Fix rpc not transferring warnings to the client Nov 17, 2025
@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch 2 times, most recently from cce0ebf to 4c08842 Compare November 17, 2025 17:01
@ElectreAAS
Copy link
Collaborator Author

ElectreAAS commented Nov 17, 2025

The fact that it looked like we solved the promotion database clearing bug was due to weird races happening. I changed the test to make it clear that we are testing the warnings, not this other behaviour.

Alizter
Alizter reviewed Nov 18, 2025
View reviewed changes
@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch 2 times, most recently from 6d924e4 to 3b007fc Compare November 18, 2025 13:45
ElectreAAS
ElectreAAS commented Nov 18, 2025
View reviewed changes
@ElectreAAS ElectreAAS changed the title Fix rpc not transferring warnings to the client Fix rpc not transferring promotion warnings to the client Nov 19, 2025
@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch from 3b007fc to a24e500 Compare November 19, 2025 17:22
@ElectreAAS
Copy link
Collaborator Author

ElectreAAS commented Nov 19, 2025 •
edited
Loading

We did a pass with @Alizter yesterday (thanks again btw) to not break the hashes in test/expect-tests/dune_rpc/dune_rpc_tests.ml, which required outputting a new version of the diagnostics RPC, which is rather verbose. This is necessary to maintain backward compatibility.
Now the hashes aren't broken, we just have a v3.
This is ready for what I hope is the final round of review

@Alizter Alizter requested review from Alizter and rgrinberg November 19, 2025 17:37
Alizter
Alizter reviewed Nov 19, 2025
View reviewed changes
bin/rpc/rpc_common.ml Outdated
(match lock_held_by with
| Dune_util.Global_lock.Lock_held_by.Unknown -> ""
| Pid_from_lockfile pid -> sprintf " (pid: %d)" pid)
|> Pp.tag User_message.Style.Warning
Copy link
Collaborator

@Alizter Alizter Nov 19, 2025

Choose a reason for hiding this comment

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

We already add the tag in User_warning.emit for the Warning: text. Otherwise this entire message will be purple which I don't think we want.

@ElectreAAS
Added test showcasing the lack of RPC warning on missing promotion ta…
e4e1d47 
…rget

Signed-off-by: Ambre Austen Suhamy <[email protected]>
@ElectreAAS
Fix promotion not having warnings
301d290 
Signed-off-by: Ambre Austen Suhamy <[email protected]>
@ElectreAAS
Adjust comment in the test to reflex bug fix.
fc371ca 
Signed-off-by: Ambre Austen Suhamy <[email protected]>
@ElectreAAS
Add sleep after build to avoid races.
d584f2f 
Signed-off-by: Ambre Austen Suhamy <[email protected]>
@ElectreAAS
Use a warning constructor instead of modifying 15 files
c22a9b1 
Signed-off-by: Ambre Austen Suhamy <[email protected]>
@ElectreAAS
Change wording in test: we only check that the warnings are correct.
ddb74e2 
Signed-off-by: Ambre Austen Suhamy <[email protected]>
@ElectreAAS
Parse alerts as alerts directly
8e2f1c6 
Signed-off-by: Ambre Austen Suhamy <[email protected]>
@ElectreAAS ElectreAAS force-pushed the fix-rpc-promotion-too-optimistic branch from a24e500 to 8e2f1c6 Compare November 24, 2025 14:09
@shonfeder shonfeder requested a review from Alizter November 26, 2025 16:38
@shonfeder
Copy link
Member

shonfeder commented Nov 26, 2025

@Alizter are you OK with the state of this? Anything needed for a ✔️

@Alizter
Copy link
Collaborator

Alizter commented Nov 26, 2025

I will give it another review.

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

Reviewers

@shonfeder shonfeder shonfeder left review comments

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

@Alizter Alizter Awaiting requested review from Alizter

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dune promote doesn't fail when not available in the promotion database dune promote + watchmode clears promotion database

4 participants

@ElectreAAS @Alizter @shonfeder @rgrinberg