Skip to content

Conversation

cablehead
Copy link
Contributor

@cablehead cablehead commented Jun 26, 2025

This PR also removes magic terminology since we no longer refer to the endpoint as a magic endpoint.

Unit tests have been added. For a manual smoke test:

### 1. Stand up a dummy Unix-socket “echo” server

This will listen on `/tmp/backend.sock` and echo back whatever you send it:

```bash
socat UNIX-LISTEN:/tmp/backend.sock,reuseaddr,fork EXEC:'/bin/cat'
```

Leave that running in Terminal A.

---

### 2. Run your new `listen-unix` side

Point it at the socket you just created:

```bash
dumbpipe listen-unix --socket-path /tmp/backend.sock
```

You’ll see something like:

```
Forwarding incoming requests to '/tmp/backend.sock'.
To connect, use e.g.:
dumbpipe connect-unix --socket-path /tmp/client.sock <NODE_TICKET>
```

Copy the `<NODE_TICKET>`.

---

### 3. Run your new `connect-unix` side

In Terminal C, do:

```bash
dumbpipe connect-unix --socket-path /tmp/client.sock <NODE_TICKET>
```

Now you’ve got a local socket at `/tmp/client.sock` that—behind the scenes—is piping straight through to your echo server.

---

### 4. Verify it works

In Terminal D, send a test string through the client socket and watch it come back:

```bash
echo "hello world" | socat - UNIX-CONNECT:/tmp/client.sock
# you should see: hello world
```

@n0bot n0bot bot added this to iroh Jun 26, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Jun 26, 2025
@cablehead cablehead marked this pull request as ready for review June 26, 2025 20:53
@Arqu
Copy link
Collaborator

Arqu commented Jun 27, 2025

clippy got a bump late last night :/

@rklaehn
Copy link
Contributor

rklaehn commented Jun 27, 2025

Thanks. Looks good on first glance.

src/main.rs Outdated
tracing::info!("derp url is {:?}", ticket.node_addr().relay_url);

// handle a new incoming connection on the magic endpoint
async fn handle_magic_accept(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove magic terminology from this, since we no longer refer to the endpoint as a magic endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a pass through, and removed references to magic terminology

1/ hope matches matches what you were thinking?
2/ hopefully it's not overloading this patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been thinking magicpipe would be a better name than dumbpipe for this utility though. It really feels like magic!

@rklaehn
Copy link
Contributor

rklaehn commented Jun 27, 2025

Very nice. Many of the code changes are just to make clippy happy and generic DRY. The code itself is nicely isolated. I don't see why we wouldn't merge this.

One thing: it would be nice to have instructions how to test this locally. I just want to give it a very basic manual smoke test.

@cablehead cablehead marked this pull request as draft June 27, 2025 17:28
@cablehead
Copy link
Contributor Author

clippy got a bump late last night :/

I can't for the life of me work out how CI is using clippy, I can't see these locally :) It might just take a few pushes to catch them all 🙏

@cablehead
Copy link
Contributor Author

cablehead commented Jun 27, 2025

One thing: it would be nice to have instructions how to test this locally. I just want to give it a very basic manual smoke test.

I've updated the README to document the new commands.

This is a quick screencast I posted to the discord running a smoke test:

dumbpipe-unix.mp4

@cablehead cablehead marked this pull request as ready for review June 27, 2025 18:21
@cablehead
Copy link
Contributor Author

Hoping that last push will address the flakiness 🤞

@cablehead
Copy link
Contributor Author

Looks like a flake in connect_listen_ctrlc_connect? (unrelated to this PR)

@Arqu
Copy link
Collaborator

Arqu commented Jul 9, 2025

Yup seems to have flaked. @rklaehn Can I ask you to do a one over on this.

@cablehead
Copy link
Contributor Author

Anything I can do to help this one get over the line?

Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

Sorry about dropping the ball on this a bit. I was traveling a lot.

The code seems to be pretty well separated, doesn't add any additional deps, so works for me.

Not so sure about the zellij example as an example, maybe we can come up with something more lightweight. But that can happen in a subsequent PR.

@rklaehn rklaehn merged commit da8754d into n0-computer:main Jul 29, 2025
14 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Jul 29, 2025
@cablehead
Copy link
Contributor Author

Thanks for the merge!

Not so sure about the zellij example as an example, maybe we can come up with something more lightweight. But that can happen in a subsequent PR.

That makes sense. If you think of a lighter example, tag me in an issue and I can PR. I'll try and think of a lighter one too.

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

Successfully merging this pull request may close these issues.

3 participants