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

refactor!: remove default protocols and iroh-cli #2928

Merged
merged 17 commits into from
Nov 25, 2024

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Nov 14, 2024

Description

As previously announced and planned, this removes all protocol implementations from the iroh crate, as well as removes the iroh-cli crate.

The core functionality of those protocols, including RPC & CLI now is in the individual crates.

Remove from iroh

  • iroh-docs
  • iroh-blobs
  • iroh-gossip

Examples

Tests

  • Move the remaining tests to the appropriate repos

CLI

  • Remove

Breaking Changes

  • iroh-cli removed
    • the crate itself
    • the ability to run iroh itself in a docker container, as there is no binary anymore
  • removed
    • iroh::blobs use iroh_blobs
    • iroh::docs use iroh_docs
    • iroh::gossip use iroh_gossip
    • iroh::client::blobs use iroh_blobs::rpc::client. a memory client is available on Blobs
    • iroh::client::tags use iroh_blobs::rpc::client. a memory client is available on Blobs
    • iroh::client::gossip use iroh_gossip::rpc::client. a memory client is available on Gossip
    • iroh::client::docs use iroh_docs::rpc::client. a memory client is available on Docs
    • iroh::client::authors use iroh_docs::rpc::client. a memory client is available on Docs
    • iroh::node::MemNode, use Node directly
    • iroh::node::FsNode, use Node directly
    • iroh::node::Node::local_pool_handle
    • iroh::node::builder::DocsStorage
    • iroh::node::builder::Builder::enable_gc_policy
    • iroh::node::builder::Builder::enable_docs
    • iroh::node::builder::Builder::register_cb_done
    • iroh::node::builder::ProtocolBuilder::local_pool_handle
    • iroh::node::builder::GcPolicy
    • iroh::util::progress
    • iroh::util::path::IrohPaths::BaoStoreDir
    • iroh::util::path::IrohPaths::DocsDatabase
    • iroh::util::path::IrohPaths::Console
    • iroh::util::path::IrohPaths::DefaultAuthor
  • iroh::node::Builder has no more generic parameters anymore
  • iroh::node::Node has no more generic parameters anymore

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@dignifiedquire dignifiedquire added this to the v0.29.0 milestone Nov 18, 2024
@dignifiedquire dignifiedquire changed the title refactor(iroh): remove external protocols from iroh refactor(iroh)!: remove external protocols from iroh Nov 19, 2024
@dignifiedquire dignifiedquire changed the title refactor(iroh)!: remove external protocols from iroh refactor!: remove default protocols and iroh-cli Nov 19, 2024
@Arqu
Copy link
Collaborator

Arqu commented Nov 19, 2024

So what's the plan now. Ideally we will set up an iroh-net based example which just tests the connectivity & pipes and we plug that back into netsim and sort out the blobs story on its own.

Also the docker files should not be removed as we still build images for iroh-relay and iroh-dns-server which are arguably much more important anyways. I just need to adapt the images & CI.

Copy link

github-actions bot commented Nov 21, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2928/docs/iroh/

Last updated: 2024-11-25T20:39:00Z

@dignifiedquire dignifiedquire self-assigned this Nov 25, 2024
dignifiedquire and others added 13 commits November 25, 2024 16:40
## Description

Moves our netsim tests to run from an example and only test the lib part
of the code, not the full CLI & blobs.

Things left to do:
- [x] rename the example from `new` to anything that makes more sense
`netsim` runs it under `iroh-transfer`
- [x] clean up the example code
- [x] add the option to provide either a relay config or pass in at
least the relay url as an argument
- [x] continue the CI adjustment and move all invocations of the netsim
runner to run from `iroh_v2` and `integration_v2` sims (we want a less
abrupt netsim switchover)
- [x] convert the remaining sims in `chuck/netsim/sims` to the new
format
- [ ] after some time flip back the CI invocations to be regular `iroh`
and `integration` sims which includes doing the same on `netsim` and
removing the old ones

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [ ] Self-review.
- [ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [ ] All breaking changes documented.
@dignifiedquire dignifiedquire marked this pull request as ready for review November 25, 2024 15:47
where
S: Store,
{
async fn run(builder: iroh::node::Builder) -> anyhow::Result<()> {
let node = builder
.enable_rpc()
Copy link
Contributor

Choose a reason for hiding this comment

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

I was a bit confused at first what node does now. I thought the idea was that iroh is a pure reexport crate and whenever you build a node like thing you DIY it using the router API. E.g. this enable_rpc enables rpc, but only for the node api, which is almost never what people want if they want rpc.

Is the plan to change this at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that Node will go away, as a follow up to this PR

@@ -0,0 +1,324 @@
use std::{
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really an example but a bench, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The intent is for it to be example that we also happen to use for netsim. It benches nothing on it's own though.

dns_resolver: Option<DnsResolver>,
node_discovery: DiscoveryConfig,
docs_storage: DocsStorage,
#[cfg(any(test, feature = "test-utils"))]
insecure_skip_relay_cert_verify: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

This mirrors all the options of the endpoint API. I found that somewhat tedious. Have you considered starting this builder with an endpoint builder?

let ep_builder = Endpoint::builder().insecure_skip_relay_cert_verify(true); // do not not call build yet
Node::builder(ep_builder).node_specific_stuff().build().await?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good idea, but this builder will be deleted in a follow up anyway

Copy link

github-actions bot commented Nov 25, 2024

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: b77d50c

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.

All my reservations are about code that is going to be removed anyway. Also, there seems to be a lot of reformatting of yaml files.

So let's get this over with...

@dignifiedquire
Copy link
Contributor Author

Also, there seems to be a lot of reformatting of yaml files.

yeah sorry, fighting my temporary editor..

@Arqu
Copy link
Collaborator

Arqu commented Nov 25, 2024

Also dropped the required Docker test as it's no longer with us ☠️

@dignifiedquire dignifiedquire added this pull request to the merge queue Nov 25, 2024
Merged via the queue into main with commit a956319 Nov 25, 2024
25 of 26 checks passed
@dignifiedquire dignifiedquire deleted the refactor-drop-external-protocols branch November 28, 2024 10:10
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