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

create a new hc command to run a holochain webrtc signal server #2265

Merged
merged 11 commits into from
Apr 25, 2023

Conversation

neonphog
Copy link
Contributor

@neonphog neonphog commented Apr 21, 2023

Summary

Adds a new hc signal-srv command to run a local holochain webrtc signal server that can be passed into a command like hc sandbox generate network webrtc ws://127.0.0.1:xxx.

TODO:

  • CHANGELOG(s) updated with appropriate info
  • Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

@neonphog neonphog marked this pull request as ready for review April 24, 2023 19:45
@neonphog neonphog requested review from steveej, ThetaSinner, maackle and jost-s and removed request for steveej and ThetaSinner April 24, 2023 19:51
@@ -38,7 +38,7 @@ page_size = "0.4.2"
parking_lot = "0.10"
rand = "0.8.5"
r2d2 = "0.8"
r2d2_sqlite = "0.21"
r2d2_sqlite = { version = "0.1", package = "r2d2_sqlite_neonphog" }
Copy link
Contributor

Choose a reason for hiding this comment

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

What's changing here going from the original package to this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ThetaSinner Alas, r2d2 is now unmaintained... i needed to update a dependency, so I published a fork of it. We can evaluate if we want to switch to a different dependency / or just roll our own connection pool at a later date.

Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Looks all groovy. For Tryorama it'll be the default to not configure the signal server and have the default address be passed to any generated sandboxes.

crates/hc_signal_srv/Cargo.toml Show resolved Hide resolved
crates/hc_signal_srv/src/lib.rs Outdated Show resolved Hide resolved
crates/hc_signal_srv/src/lib.rs Show resolved Hide resolved
crates/hc_signal_srv/src/lib.rs Show resolved Hide resolved
crates/hc_signal_srv/src/lib.rs Outdated Show resolved Hide resolved
neonphog and others added 2 commits April 24, 2023 16:12
Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>
Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>
@neonphog
Copy link
Contributor Author

@jost-s

Looks all groovy. For Tryorama it'll be the default to not configure the signal server and have the default address be passed to any generated sandboxes.

Not sure I follow the "not configure the signal server" part. If you don't specify any configuration, you'll get an ephemerally assigned port, which is good, because it'll always work, but then you will need to pass that into the sandboxes, otherwise they won't know what signal port to connect to.

@jost-s
Copy link
Contributor

jost-s commented Apr 24, 2023

Not sure I follow the "not configure the signal server" part. If you don't specify any configuration, you'll get an ephemerally assigned port, which is good, because it'll always work, but then you will need to pass that into the sandboxes, otherwise they won't know what signal port to connect to.

Sorry, yes, the port has to be set or parsed from the output. Parsing should be fine.

@neonphog
Copy link
Contributor Author

@jost-s

Parsing should be fine

I also added a --address-list-file-path parameter that will write the list of interface addresses bound to a file thinking that might be easier to read than stdout.

@jost-s
Copy link
Contributor

jost-s commented Apr 24, 2023

@jost-s

Parsing should be fine

I also added a --address-list-file-path parameter that will write the list of interface addresses bound to a file thinking that might be easier to read than stdout.

Yes, I tested it all =D True, that'll be more convenient 👍

Copy link
Member

@maackle maackle left a comment

Choose a reason for hiding this comment

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

Looks good aside from the naming possibilities which I'm not holding too strongly for

@@ -165,6 +166,8 @@ pub enum Opt {
WebApp(hc_bundle::HcWebAppBundle),
/// Work with sandboxed environments for testing and development
Sandbox(hc_sandbox::HcSandbox),
/// Run a signal server
Copy link
Member

@maackle maackle Apr 25, 2023

Choose a reason for hiding this comment

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

Suggested change
/// Run a signal server
/// Run a WebRTC signaling server

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe

Suggested change
/// Run a signal server
/// Run a WebRTC signaling server

@@ -165,6 +166,8 @@ pub enum Opt {
WebApp(hc_bundle::HcWebAppBundle),
/// Work with sandboxed environments for testing and development
Sandbox(hc_sandbox::HcSandbox),
/// Run a signal server
SignalSrv(hc_signal_srv::HcSignalSrv),
Copy link
Member

Choose a reason for hiding this comment

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

hc signal-srv is not a great look IMO. I only know what a signal server is because I've followed your WebRTC work somewhat, but if I didn't, I don't know if I'd know how to interpret "signal-srv". I prefer hc signal-server but I wouldn't block this on the naming.

Structopt allows users to type the shortest unambiguous prefix, so for those who love brevity they can still just type hc sig either way.

Btw, searching for "signal server", the main hits are about the Signal messenger protocol, so it's probably good to qualify this as "WebRTC signal server" whenever possible in high visibility places where new users might engage with it.

[edit] searching for "webrtc signal server" shows that it's actually officially called a "signaling server". So 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I would've preferred something else than signal-srv too! =D Sounds like "signal surf"!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jost-s @maackle - feel free to change it. Rust embraces short-cut names (fn, etc), I feel like we should have shortened it further to just hc sig lol : ), but I have zero attachment to the actual name.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about hc ss? :D

I'm not fond of the abbreviated names in Rust and was surprised to find them to be common. Long identifiers are cheap with code completion and compilation to byte code. For me it's more a general thing with that than specific to this case. If you know about webrtc you'll intuit signal-srv, and if you don't you probably won't. But signal-server doesn't explain you what it does either. So it should be hc wss for webrtc signal server ; )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'course wss means Web-Sockets-Secure : )

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it was purely meant as a joke.

use tx5_signal_srv::Result;

#[derive(Debug, StructOpt)]
/// Helper for running a holochain webrtc signal server.
Copy link
Member

@maackle maackle Apr 25, 2023

Choose a reason for hiding this comment

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

Suggested change
/// Helper for running a holochain webrtc signal server.
/// Helper for running a Holochain WebRTC signaling server.

let (driver, addr_list, err_list) = tx5_signal_srv::exec_tx5_signal_srv(config)?;

for err in err_list {
println!("# HC SIGNAL SRV - ERROR: {err:?}");
Copy link
Member

Choose a reason for hiding this comment

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

If you agree to the other change

Suggested change
println!("# HC SIGNAL SRV - ERROR: {err:?}");
println!("# HC SIGNAL SERVER - ERROR: {err:?}");

@maackle maackle mentioned this pull request Apr 25, 2023
2 tasks
@neonphog neonphog merged commit ae84064 into develop Apr 25, 2023
8 of 12 checks passed
@neonphog neonphog deleted the hc-signal-srv branch April 25, 2023 22:02
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

4 participants