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: use custom wait for watching/helpers.sh #8201

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Jul 13, 2023

This allows the fs-memo.t test to succeed on Nix. Before the pid of Dune was being thrown away by a bash bug causing wait to think it was not a child process and rejecting it. We therefore wait instead for all child processes.

@Alizter Alizter mentioned this pull request Jul 13, 2023
11 tasks
@ocaml-benchmarks
Copy link

#8201 (40242a5) changes the metrics as follows in comparison to main (a0145b2) when running on fermat (bench/monorepo/bench.Dockerfile):

Benchmark: default

Test: dune monorepo benchmarks

metric_name Last PR value Last main value Delta
build from scratch 1132.841449 1129.820496 0.3%
null build 69.014337 65.869692 4.8%
watch mode: changing file in 'base' library 70.042814 69.897738 0.2%
watch mode: changing file in 'file_path' library 18.019483 18.689723 -3.6%
watch mode: fixing error in file in 'base' library 62.521988 63.562635 -1.6%
watch mode: fixing error in file in 'file_path' library 17.520375 17.633133 -0.6%
watch mode: introducing error in file in 'base' library 8.000441 8.154346 -1.9%
watch mode: introducing error in file in 'file_path' library 2.538723 2.827425 -10.2%

@Alizter Alizter requested a review from rgrinberg July 14, 2023 11:58
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.

Amazing if this will fix the ./helpers.sh: line 25: wait: pid 79252 is not a child of this shell bug I get on every run.

I was implementing something similar (sleep in a loop) but maybe it would be useful to automatically abort after a certain amount of time/iterations have passed to prevent e.g. CI instances from busywaiting.

@Alizter
Copy link
Collaborator Author

Alizter commented Jul 18, 2023

@Leonidas-from-XIV The issue with that is it will be very system dependent. Something that takes 1s on my machine may take a lot longer on another. I chose the .1s value on purpose so to not be too slow and not keep the OS busy.

@Leonidas-from-XIV
Copy link
Collaborator

@Alizter The problem is that if there is no other cancellation it will busy-wait forever (and unlike wait, take up CPU time) even if there is no chance it will finish. My suggestion is to e.g. add a counter and add to it each iteration and if it reaches, say, 6000 (6000 * 0.1s = 600s = 10min) abort with a failure, that way the test will terminate somewhat reliably if there is an issue.

@Alizter Alizter force-pushed the ps/branch/test__use_custom_wait_for_watching_helpers_sh branch from 2fbe0ae to 8ca17e0 Compare July 19, 2023 13:18
@Alizter Alizter force-pushed the ps/branch/test__use_custom_wait_for_watching_helpers_sh branch from 8ca17e0 to f7cf0e8 Compare July 23, 2023 22:57
@Alizter
Copy link
Collaborator Author

Alizter commented Jul 23, 2023

@rgrinberg I didn't know that /proc wasn't available outside of linux. I've changed it now to wait on all children processes. This now works on Nix and should be MacOS compatible.

@Leonidas-from-XIV There is already a timeout with this command it seems, so I've changed it back to the regular wait.

@Alizter Alizter force-pushed the ps/branch/test__use_custom_wait_for_watching_helpers_sh branch from b330a92 to 563a4c3 Compare July 30, 2023 13:15
@Alizter Alizter requested a review from rgrinberg July 30, 2023 13:30
@Alizter Alizter force-pushed the ps/branch/test__use_custom_wait_for_watching_helpers_sh branch from 563a4c3 to 285a1e7 Compare July 30, 2023 14:18
@pmwhite
Copy link
Collaborator

pmwhite commented Jul 30, 2023

I just opened #8303 to fix this same issue, but then closed it because this already exists. I don't have a preference either way, but in case it hasn't already been considered, switching from wait to tail fixes the problem as well. I don't know if it is cross-platform enough, though - I'm just running the tests on a Linux machine.

if [ "$(uname -s)" = "Linux" ]
then
# wait for all child processes
wait $(jobs -p);
Copy link
Member

Choose a reason for hiding this comment

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

Could you use phil's tail hack instead of this. it seems like it's a more faithful emulation of waiting for $pid than waiting for all jobs.

@rgrinberg
Copy link
Member

I don't know if it is cross-platform enough, though - I'm just running the tests on a Linux machine.

It's not. But it's the better workaround on Linux

This allows the fs-memo.t test to succeed on Nix. Before the pid of Dune
was being thrown away by a bash bug causing wait to think it was not a
child process and rejecting it. We therefore wait instead for all child
processes.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/branch/test__use_custom_wait_for_watching_helpers_sh branch from 7011259 to 8fb2d30 Compare August 3, 2023 14:43
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 3, 2023

@rgrinberg I've used tail instead now. Works on my machine.

@rgrinberg rgrinberg merged commit bb86a4a into ocaml:main Aug 3, 2023
@Alizter Alizter deleted the ps/branch/test__use_custom_wait_for_watching_helpers_sh branch August 4, 2023 10:48
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.

4 participants