Skip to content

Allow concurrent exec with watch mode #11840

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
Merged
merged 1 commit into from
Jun 19, 2025
Merged

Allow concurrent exec with watch mode #11840

merged 1 commit into from
Jun 19, 2025

Conversation

gridbugs
Copy link
Collaborator

This change allows a limited version of dune exec to run at the same time as dune is running in watch mode. This allows users to run example programs without needing to stop their watch server. This works by sending messages to the RPC server to build the executable if necessary.

@gridbugs gridbugs force-pushed the exec-concurrent-with-watch-mode branch from 1d6ea1e to 09a0dbe Compare May 23, 2025 06:58
@Leonidas-from-XIV
Copy link
Collaborator

Thanks for the PR, it'll be easier to review once #11833 is merged.

@gridbugs gridbugs changed the title Allow concurrent with watch mode Allow concurrent exec with watch mode May 26, 2025
@gridbugs gridbugs force-pushed the exec-concurrent-with-watch-mode branch from 09a0dbe to 63481de Compare May 27, 2025 06:38
@gridbugs
Copy link
Collaborator Author

I've added a bunch of #11833 into this PR. Once #11833 is merged this diff will get much smaller.

@gridbugs gridbugs force-pushed the exec-concurrent-with-watch-mode branch 3 times, most recently from f3b6fe0 to 171c7a3 Compare June 2, 2025 06:36
@Leonidas-from-XIV
Copy link
Collaborator

@gridbugs Can you rebase, now that #11833 is merged?

@gridbugs
Copy link
Collaborator Author

gridbugs commented Jun 9, 2025

Will do. I was waiting for #11879 to be merged first as it changes a lot of code in exec.ml.

@gridbugs gridbugs force-pushed the exec-concurrent-with-watch-mode branch 2 times, most recently from 32a0082 to b91cfa6 Compare June 11, 2025 07:45
@gridbugs
Copy link
Collaborator Author

@Leonidas-from-XIV all the prereqs for this are now merged. Can you please take a look.

Leonidas-from-XIV
Leonidas-from-XIV requested changes Jun 11, 2025
View reviewed changes
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Very useful work, however I am concerned about the maintainability if the tests are disabled for being flaky. Is there maybe some way to make them more stable?

Alizter
Alizter reviewed Jun 11, 2025
View reviewed changes
test/blackbox-tests/test-cases/watching/watching-eager-concurrent-exec-command.t
Hello, World!

$ dune shutdown
$ wait
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we waiting at the end?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My intention is to wait for the background process started by dune build README.md --watch & to terminate before exiting the test.

@gridbugs gridbugs force-pushed the exec-concurrent-with-watch-mode branch from b91cfa6 to 1254b46 Compare June 12, 2025 03:03
@gridbugs
Copy link
Collaborator Author

Very useful work, however I am concerned about the maintainability if the tests are disabled for being flaky. Is there maybe some way to make them more stable?

Flaky watch-mode tests is something that has plagued dune for as long as I've worked on it and I'm not sure what exactly we can do about it. @Alizter I recall you doing some work on this recently?

@Alizter
Copy link
Collaborator

Alizter commented Jun 12, 2025

@gridbugs I fixed it somewhat with #11827 and then it was decided a more general approach as in #11841 would be a better fit. The issue in the latter is that the CI is still a bit flaky for some reason.

@gridbugs gridbugs force-pushed the exec-concurrent-with-watch-mode branch 2 times, most recently from 391ac6a to 2855ca4 Compare June 17, 2025 07:08
Leonidas-from-XIV
Leonidas-from-XIV reviewed Jun 17, 2025
View reviewed changes
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

My only remaining complaint is that I don't like how the utility functions controls the output prettyprinting of the type, I believe the way that the PID is printed should be done in the bin/* modules.

@gridbugs gridbugs force-pushed the exec-concurrent-with-watch-mode branch 2 times, most recently from 4bcb45d to 857372c Compare June 18, 2025 05:22
@gridbugs
Allow dune exec while watch server is running
5e8dbb9 
This change allows a limited version of `dune exec` to run at the same
time as dune is running in watch mode. This allows users to run example
programs without needing to stop their watch server. This works by
sending messages to the RPC server to build the executable if necessary.

Signed-off-by: Stephen Sherratt <[email protected]>
@gridbugs gridbugs force-pushed the exec-concurrent-with-watch-mode branch from 857372c to 5e8dbb9 Compare June 19, 2025 11:18
@gridbugs gridbugs merged commit c6f8143 into ocaml:main Jun 19, 2025
22 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Leonidas-from-XIV Leonidas-from-XIV Leonidas-from-XIV approved these changes

@Alizter Alizter Alizter approved these changes

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

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

Successfully merging this pull request may close these issues.
4 participants
@gridbugs @Leonidas-from-XIV @Alizter @rgrinberg