rpc status: fix for Win32 by nojb · Pull Request #8806 · ocaml/dune

nojb · 2023-09-30T22:19:11Z

Beginning of a fix for #8799, but not complete because dune rpc status does not seem to be fully working:

(no data is reported for the connected client)

cc @Alizter

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

nojb · 2023-09-30T22:44:30Z

After further testing, it seems to be working:

@Alizter, can you give it a try on your side to confirm? Thanks!

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Alizter

Works great, thanks!

cc @emillon any chance we can get this backported for 3.9-3.11? Or at least add it to a list in case another point release is decided.

rgrinberg · 2023-10-01T03:41:12Z

otherlibs/dune-rpc/private/where.ml

-        | Error e -> Error e
-        | Ok contents ->
-          (match of_string contents with
+      (match default ~build_dir () with


I don't know what the issue is yet, but this fix doesn't seem right to me. Previously, the code would look at the socket inside the build directory to connect, and now it's just assuming the address is the default address.

Nothing has changed here for Unix systems: default ~build_dir () still returns the socket inside the build directory:

dune/otherlibs/dune-rpc/private/where.ml

Lines 130 to 134 in 071cb47

let default ?(win32 = win32) ~build_dir () =

if win32

then `Ip (`Host (Unix.string_of_inet_addr Unix.inet_addr_loopback), `Port default_port)

else `Unix (Filename.concat build_dir rpc_socket_relative_to_build_dir)

;;

Okay, but even on Windows, the workflow to connect should be:

Inspect the file _build/.rpc/dune

If it's a socket, connect to it (will never happen on windows)

If it's a normal file, read it and get the connection string from here. On windows, that connection string will have the address we want to connect on.

Oops, you are right. I hadn't grasped the logic behind the client/server split. Now I pushed a commit with the right fix I think.

nojb · 2023-10-01T16:53:45Z

src/dune_rpc_client/where.ml

-  | `Unix p -> sprintf "unix://%s" p
-  | `Ip (`Host host, `Port port) -> sprintf "%s:%d" host port
-;;
+let to_string t = Dune_rpc_private.Where.to_string t


Use the right "dbus" format so that it can be parsed back in

dune/otherlibs/dune-rpc/private/where.ml

Lines 154 to 162 in 071cb47

let of_file f =

let+ contents = IO.read_file f in

match contents with

| Error e -> Error e

| Ok contents ->

(match of_string contents with

| Error e -> Error e

| Ok s -> Ok (Some s))

in

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

rgrinberg · 2023-10-01T17:05:50Z

doc/changes/8806.md

@@ -0,0 +1 @@
+- Fix `dune rpc status` on Windows (#8806, fixes #8799, @nojb)


This fixes a little more than status. It should fix all dune rpc commands in fact.

Looks like monitor too.

Thanks for the review. I amended the Changes entry.

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

emillon · 2023-10-02T07:01:45Z

Works great, thanks!

cc @emillon any chance we can get this backported for 3.9-3.11? Or at least add it to a list in case another point release is decided.

Sure. To be clear, this is a regression introduced in 3.9.0?

nojb · 2023-10-02T07:03:08Z

Works great, thanks!
cc @emillon any chance we can get this backported for 3.9-3.11? Or at least add it to a list in case another point release is decided.

Sure. To be clear, this is a regression introduced in 3.9.0?

I suspect this has never worked on Windows, until this PR.

emillon · 2023-10-02T07:08:58Z

OK. Note a huge rush then. I'll backport that to 3.11.1.

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com> Signed-off-by: Etienne Millon <me@emillon.org>

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com> Signed-off-by: Etienne Millon <me@emillon.org> Co-authored-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

@nojb

CHANGES: - Fix `dune rpc` commands on Windows (ocaml/dune#8806, fixes ocaml/dune#8799, @nojb) - Fix `inline_tests` when the partition list is empty (ocaml/dune#8849, fixes ocaml/dune#8848, @hhugo)

@nojb

CHANGES: - Fix `dune rpc` commands on Windows (ocaml/dune#8806, fixes ocaml/dune#8799, @nojb) - Fix `inline_tests` when the partition list is empty (ocaml/dune#8849, fixes ocaml/dune#8848, @hhugo)

rpc status: fix for Win32

e7e8c50

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

nojb force-pushed the rpc_status_win32 branch from f4b5978 to e7e8c50 Compare September 30, 2023 22:33

Changes

6642e45

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Alizter approved these changes Sep 30, 2023

View reviewed changes

rgrinberg requested changes Oct 1, 2023

View reviewed changes

nojb commented Oct 1, 2023

View reviewed changes

Better fix

34c39d6

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

nojb force-pushed the rpc_status_win32 branch from 719d192 to 34c39d6 Compare October 1, 2023 16:57

rgrinberg approved these changes Oct 1, 2023

View reviewed changes

nojb and others added 4 commits October 1, 2023 18:15

Changes

f8ecb77

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Merge branch 'main' into rpc_status_win32

bd85dab

Accept

9292675

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Merge branch 'main' into rpc_status_win32

9507130

nojb merged commit c91a9f6 into ocaml:main Oct 1, 2023

nojb deleted the rpc_status_win32 branch October 1, 2023 18:28

rgrinberg added the upstream-sync label Oct 1, 2023

emillon mentioned this pull request Oct 2, 2023

3.11.1 release tracking #8810

Closed

7 tasks

rgrinberg linked an issue Oct 2, 2023 that may be closed by this pull request

windows: dune rpc status not working #8799

Closed

emillon pushed a commit to emillon/dune that referenced this pull request Oct 9, 2023

Fix dune rpc commands on Windows (ocaml#8806)

63d0dae

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

emillon pushed a commit to emillon/dune that referenced this pull request Oct 9, 2023

Fix dune rpc commands on Windows (ocaml#8806)

07f72a8

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com> Signed-off-by: Etienne Millon <me@emillon.org>

emillon added a commit that referenced this pull request Oct 9, 2023

Fix dune rpc commands on Windows (#8806) (#8892)

a760f8c

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com> Signed-off-by: Etienne Millon <me@emillon.org> Co-authored-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

emillon mentioned this pull request Oct 9, 2023

[new release] dune (15 packages) (3.11.1) ocaml/opam-repository#24607

Merged

	let default ?(win32 = win32) ~build_dir () =
	if win32
	then `Ip (`Host (Unix.string_of_inet_addr Unix.inet_addr_loopback), `Port default_port)
	else `Unix (Filename.concat build_dir rpc_socket_relative_to_build_dir)
	;;

	let of_file f =
	let+ contents = IO.read_file f in
	match contents with
	\| Error e -> Error e
	\| Ok contents ->
	(match of_string contents with
	\| Error e -> Error e
	\| Ok s -> Ok (Some s))
	in

		@@ -0,0 +1 @@
		- Fix `dune rpc status` on Windows (#8806, fixes #8799, @nojb)

Conversation

nojb commented Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Sep 30, 2023

Uh oh!

Alizter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emillon commented Oct 2, 2023

Uh oh!

nojb commented Oct 2, 2023

Uh oh!

emillon commented Oct 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nojb commented Sep 30, 2023 •

edited

Loading