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: reproduce dune exec -w crash with pkg management #10960

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

Conversation

anmonteiro
Copy link
Collaborator

reproduces the crash in #10959

@Alizter
Copy link
Collaborator

Alizter commented Sep 29, 2024

  let update_build_progress_exn ~f =

is initialising when this happens so I need to find who is updating the build progress this early.

@Alizter
Copy link
Collaborator

Alizter commented Sep 29, 2024

For some reason this stems from the let* binding of env in run_eagar_watch. Not sure how it is being triggered yet however.

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
Copy link
Collaborator Author

I just pushed a commit that delays evaluating Env.t Memo.t until get_path_and_build_if_necessary

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

$ dune exec -w ./x.exe > output.log 2>&1 &
$ PID=$!
$ sleep 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

2 seems long, what about 0.1? The other exec-watch tests appear to pick something like that. Although killing processes this was seems very fragile, there ought to be a more principled way to test these. Not this PRs problem however.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The watching/ tests use some better helpers. Up to you if you want to improve things. Also I would move this from pkg to exec-watch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could you point me to a relevant example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

They have these start_dune stop_dune helpers from a shared script (see the dune file for where it is). A test example is: https://github.com/ocaml/dune/blob/main/test/blackbox-tests/test-cases/watching/copy-rules.t

The logic in the script handles stopping the watch mode build and the output. Presumably something like that could be adapted for exec. But if this is too much work that is also fine, since the other exec-watch/ tests are written this way.

@@ -69,10 +69,9 @@ module Command_to_exec = struct

type t =
{ get_path_and_build_if_necessary :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps call this get_env_and_build_if_necessary.

@anmonteiro
Copy link
Collaborator Author

before addressing the outstanding code review items, I'd like some confirmation that the fix is in the right direction, or even correct.

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

Successfully merging this pull request may close these issues.

4 participants