-
Notifications
You must be signed in to change notification settings - Fork 136
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
Tx5 Holochain WebRTC Integration #1741
Conversation
@@ -58,6 +58,7 @@ rec { | |||
gh | |||
nixpkgs-fmt | |||
cargo-sweep | |||
go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveej - is this the correct place to put a new build dependency for holochain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this just provides it to the developer shell.
native toolchains that are used during the build process go here
we're in the process of switching our nix derivations for which i can't blindly suggest the place. which crate(s) will need the go binary available for its builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveej - kitsune_p2p is the direct dependent, but everything downstream of that, including holochain will obviously need it too, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hnrg... @steveej maybe it makes the most sense to wait for your refactor to finish before trying to get go into holonix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks orderly and readable to me. Are there any tests with tx5 enabled?
@jost-s - no we need some more infrastructure (TURN servers, etc) before we can start testing it... We'll need some local docker TURN servers for local testing, but that will come in a later PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions about the necessity of lair integration, and whether the two tx features are meant to be XOR or OR.
@@ -32,6 +34,8 @@ pub struct KitsuneHostImpl { | |||
ribosome_store: RwShare<RibosomeStore>, | |||
tuning_params: KitsuneP2pTuningParams, | |||
strat: ArqStrat, | |||
lair_tag: Option<Arc<str>>, | |||
lair_client: Option<lair_keystore_api::LairClient>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm kitsune needs to be integrated with lair now?
also, should these Options go together? Do they both need to be Some or None together or can they be independent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They could go together, but there are also some small options if they are separate:
- if you specify the lair_client and not the tag, you'll get a new node id each time. maybe useful?
- if you specify the tag and not the client, you'll always know which tag is the node id, even in new lair clients. maybe useful?
not sure : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm kitsune needs to be integrated with lair now?
Yeah, this is the most expeditious path forward. If we want to remove that specific dependency in the future, we can write an abstraction, but I don't think it's a high priority.
#[cfg(feature = "tx2")] | ||
{ | ||
#[cfg(feature = "tx5")] | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code requires both features tx2 and tx5, is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We only have to do the match to determine if we're in tx2 mode if tx5 is enabled, otherwise we know we're in tx2 mode. This will be different if we ever have a third option
#[cfg(feature = "tx2")] | ||
if ep_hnd.is_none() && config.is_tx2() { | ||
tracing::trace!("tx2"); | ||
let (h, e) = MetaNet::new_tx2(config.clone(), tls_config, metrics).await?; | ||
ep_hnd = Some(h); | ||
ep_evt = Some(e); | ||
} | ||
|
||
// capture endpoint handle | ||
let ep_hnd = ep.handle().clone(); | ||
#[cfg(feature = "tx5")] | ||
if ep_hnd.is_none() && config.is_tx5() { | ||
tracing::trace!("tx5"); | ||
let signal_url = match config.transport_pool.get(0).unwrap() { | ||
TransportConfig::WebRTC { signal_url } => signal_url.clone(), | ||
_ => unreachable!(), | ||
}; | ||
let (h, e) = | ||
MetaNet::new_tx5(config.tuning_params.clone(), host.clone(), signal_url).await?; | ||
ep_hnd = Some(h); | ||
ep_evt = Some(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it desired that the tx2 endpoint is clobbered if both features are enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be clobbered because of the ep_hnd.is_none()
check. So, if tx2 is enabled && the config is_tx2()
we will get a tx2 hnd.
The intention is that if both features are enabled, the conductor config will control which is used. I believe I've accomplished that : ) |
…p uninstall (#1805) (#1838) * feat(nix): fix nextest derivation for macos (#1822) * fix(github/workflows/holonix-integration): use multi-arch runner (#1826) * fix(github/workflows/holonix-integration): use multi-arch runner * fix(github/workflows/holonix-integration): make work on multi-arch * update holochain-nixpkgs on develop (#1830) update nix sources see VERSIONS.md for the exact changes Co-authored-by: Holochain Release Automation <hrabot@holochain.org> * update holochain-nixpkgs on develop (#1832) update nix sources see VERSIONS.md for the exact changes Co-authored-by: Holochain Release Automation <hrabot@holochain.org> * update holochain-nixpkgs on develop (#1833) update nix sources see VERSIONS.md for the exact changes Co-authored-by: Holochain Release Automation <hrabot@holochain.org> * feat(CHANGELOGs): facilitate bump to next minor pre-release (#1823) * Tx5 Holochain WebRTC Integration (#1741) * tx4 feature flipper * checkpoint * webrtc config * abstraction checkpoint * networking abstraction * tx4 integration checkpoint * tx4 request * checkpoint integration * merge fixes * remove local crate overrides * test fixes * run tx5 tests on CI * ci config fix * add go to ci build * add go to coreDev flavor * tx5 update * var fix * changelog * limit jobs to prevent OOM * nextest build jobs * shrink debug info * no go debug symbols; revert some unhelpful ci config * fix(ssh-session): correct nix-daemon settings (#1834) * refactor(generate-readme): switch to cargo-rdme (#1820) * docs(hdi): prepare readme and lib for cargo-rdme * docs(hdk): prepare lib and readme for cargo-rdme * docs(holochain_keystore): prepare readme for cargo-rdme * docs(holochain_state): prepare lib and readme for cargo-rdme * docs(hdk+hdi): delete obsolete tpl files * refactor(generate_readmes): replace cargo-readme with cargo-rdme * fix(hdk): intra-doc link to hdk_extern macro * ci(readme): remove cargo-readme and install cargo-rdme * ci(create-readme): configure git * build(generate-readme): unset git name & email afterwards * docs(crate-level): generate readmes from doc comments * fix clippy --------- Co-authored-by: release-ci <ci@holo.host> * update holochain-nixpkgs on develop (#1836) update nix sources see VERSIONS.md for the exact changes Co-authored-by: Holochain Release Automation <hrabot@holochain.org> * Delete authored data from db on cell cleanup after app uninstall (#1805) * Delete all authored and dht data when last cell in a space leaves * Changelog * Incorporate PR feedback * Clean up authored and dht db when space is empty of local cells * Little cleanup * build.rs checks for schema diff * Rename schema files to version 0 * Create our first db migration * Some better docs * Set schema diff to original ref, filter out additions * Fix migrations logic, add tests * Fix migration * Clippy fix * Fix panicky trace * Use transactions when applying migrations * Check against develop for schema changes * update holochain-nixpkgs on develop (#1841) update nix sources see VERSIONS.md for the exact changes Co-authored-by: Holochain Release Automation <hrabot@holochain.org> * docs(nix): add comments to nix flake (#1829) * add comments to flake * add default dev shell module * feat(nix): add dev shells to flake * update changelog * delete redundant dev shells module * undo changelog update --------- Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com> --------- Co-authored-by: DavHau <hsngrmpf+github@gmail.com> Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com> Co-authored-by: Holochain Release Automation <100725712+holochain-release-automation2@users.noreply.github.com> Co-authored-by: Holochain Release Automation <hrabot@holochain.org> Co-authored-by: David Braden <neonphog@gmail.com> Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> Co-authored-by: release-ci <ci@holo.host>
Summary
TODO: