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

fix: prompt to run mix deps.get if deps are out of sync on start #338

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

jjcarstens
Copy link
Contributor

@jjcarstens jjcarstens commented Nov 15, 2023

2023-11-15
This is just the start of a basic idea for prompting when mix dep failures occur. I'm getting the prompt, but GenLSP.request/2 is timing out which causes runtime to crash and my yes/no clicks seem to go nowhere. Timed out for now, but starting this draft in case others are able to look before me:

GenLSP timeout + Runtime crash

06:19:07.568 [error] GenServer #PID<0.689.0> terminating
** (stop) exited in: GenServer.call(NextLS.Buffer, {:outgoing_sync, %{"id" => 1730, "jsonrpc" => "2.0", "method" => "window/showMessageRequest", "params" => %{"actions" => [%{"title" => "yes"}, %{"title" => "no"}], "message" => "The NextLS runtime failed with errors on dependencies. Would you like to re-fetch them?", "type" => 1}}}, 5000)
    ** (EXIT) time out
    (elixir 1.15.7) lib/gen_server.ex:1074: GenServer.call/3
    (next_ls 0.15.0) lib/next_ls.ex:678: anonymous fn/5 in NextLS.handle_notification/2
    (next_ls 0.15.0) lib/next_ls/runtime.ex:316: NextLS.Runtime.handle_info/2
    (stdlib 5.1.1) gen_server.erl:1077: :gen_server.try_handle_info/3
    (stdlib 5.1.1) gen_server.erl:1165: :gen_server.handle_msg/6
    (stdlib 5.1.1) proc_lib.erl:241: :proc_lib.init_p_do_apply/3
Last message: {#Port<0.206>, {:data, "** (Mix) Can't continue due to errors on dependencies\n"}}
State: %{logger: #PID<0.292.0>, name: "wat", parent: ~c"g1h3KG5leHQtbHMtMTcwMDA1NDIxMjUyNTk1NjMzNEBERVZNMTA1MzlSRU0AAAFCAAAAAGVUxMU=", port: #Port<0.206>, registry: NextLS.Registry, errors: nil, task_supervisor: :runtime_task_supervisor, working_dir: "/private/tmp/wat", on_initialized: #Function<37.93096054/1 in NextLS.handle_notification/2>, compiler_refs: %{}}

image

@mhanberg
Copy link
Collaborator

Thanks for getting this started! I believe I need to change the timeout in GenLSP, as it's only waiting 5 seconds for you to click the button.

Also, that function should return with the response from the editor, so you'll need to use it. I assume that's why you say "they go nowhere"

Great work!

@jjcarstens
Copy link
Contributor Author

Ah, I hadn't dug deep and assumed the whole message cycle was going to be asynchronous. It seems like we need to handle the case where the user does nothing, ignores it, or doesn't get to the message in time. Can we tell the message to clear?

Also, I think I need to think a bit on the mix error capturing as well. There could be a legitimate error (like the dep won't compile because host tools are missing or something, etc) and we wouldn't want to prompt in that case. I think a majority of cases are solved with mix deps.get though

@mhanberg
Copy link
Collaborator

Ah, I hadn't dug deep and assumed the whole message cycle was going to be asynchronous. It seems like we need to handle the case where the user does nothing, ignores it, or doesn't get to the message in time.

I think that we most likely want the function to never time out, and if you want it to not block, you run it in a task and set a timeout on that, and handle accordingly.

Can we tell the message to clear?

i'll have to research that

@mhanberg
Copy link
Collaborator

Another thing to consider is the state of the runtime when we know there is an error with it.

Meaning, if there are errors, it should pause and wait for some direction and not just go into a restart loop

@jjcarstens
Copy link
Contributor Author

jjcarstens commented Nov 15, 2023

Another thing to consider is the state of the runtime when we know there is an error with it.

Meaning, if there are errors, it should pause and wait for some direction and not just go into a restart loop

🤔 hmm. This will take some thought as well. The mix deps.loadpath is what fails causing a non-zero exit of the private compiler port process which later gets handled and shuts things down. Is there currently a mechanism to leave the LSP and delay/retry the runtime start later?

@mhanberg
Copy link
Collaborator

It is probably acceptable to explicitly handle errors from the deps.loadpath call.

the runtime currently has some implicit state (if there is a node in the process state, it is considered "ready"), but it might be worth it to turn it into an explicit state machine (possibly using gen_statem)

@mhanberg mhanberg changed the title Initial start for handling mix dep errors fix: prompt to run mix deps.get if deps are out of sync on start Feb 28, 2024
@mhanberg mhanberg marked this pull request as ready for review February 28, 2024 00:25
@mhanberg mhanberg force-pushed the mix-deps-prompt branch 2 times, most recently from 5e86bf5 to 802f1fe Compare February 28, 2024 00:41
@mhanberg mhanberg enabled auto-merge (squash) February 28, 2024 00:42
Partially addresses elixir-tools#53 elixir-tools#115 elixir-tools#285

Co-authored-by: Mitchell Hanberg <mitch@mitchellhanberg.com>
@mhanberg mhanberg merged commit 55e91ac into elixir-tools:main Feb 28, 2024
14 checks passed
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.

None yet

2 participants