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

swarm-derive: Add prelude configuration option to NetworkBehaviour macro #3055

Merged
merged 20 commits into from
Nov 12, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 24, 2022

Description

I recommend patch-by-patch review.

Links to any relevant issues

Depends-On: #3081

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Commit message body

Currently, our NetworkBehaviour derive macro depends on the libp2p crate to be in scope. This prevents standalone usage which forces us to depend on libp2p in all our tests where we want to derive a NetworkBehaviour.

This PR introduces a prelude option that - by default - points to libp2p::swarm::derive_prelude, a new module added to libp2p_swarm. With this config option, users of libp2p_swarm can now refer to the macro without depending on libp2p, breaking the circular dependency in our workspace. For consistency with the ecosystem, the macro is now also re-exported by libp2p_swarm instead of libp2p at the same position as the trait that it implements.

Lastly, we introduce an off-by-default macros feature flag that shrinks the dependency tree for users that don't need the derive macro.

The fact that `libp2p-swarm-derive` is a separate crate is an
implementation detail and only required because `cargo` needs to
compile this crate separately. It semantically belongs to `libp2p-swarm`
so this is where the tests should be.
This allows us to depend on the macro without depending on the entire
`libp2p` crate which causes circular dependencies in our tests.
@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2022

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2022

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@thomaseizinger
Copy link
Contributor Author

Friendly ping @mxinden @jxs @elenaf9 :)

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM Thomas!

@thomaseizinger
Copy link
Contributor Author

Note that this also turns out to be a requirement for libp2p/test-plans#72.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Tiny nit pick. Otherwise this looks good to me.

/// Parses the `value` of a key=value pair in the `#[behaviour]` attribute into the requested type.
///
/// Only `String` values are supported, e.g. `#[behaviour(foo="bar")]`.
fn parse_attribute_value_by_key<T>(ast: &DeriveInput, key: &str) -> Option<T>
Copy link
Member

Choose a reason for hiding this comment

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

🙏

swarm/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Max Inden <mail@max-inden.de>
swarm/src/lib.rs Outdated Show resolved Hide resolved
@mergify mergify bot merged commit afb777e into master Nov 12, 2022
@thomaseizinger thomaseizinger deleted the 3053-move-swarm-derive branch November 17, 2022 01:17
libp2p-kad = { path = "../protocols/kad" }
libp2p-ping = { path = "../protocols/ping" }
libp2p-plaintext = { path = "../transports/plaintext" }
libp2p-swarm-derive = { version = "0.30.2", path = "../swarm-derive" }
Copy link
Member

Choose a reason for hiding this comment

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

@thomaseizinger why define libp2p-swarm-derive with a version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy-paste error :)

umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
…` macro (libp2p#3055)

Currently, our `NetworkBehaviour` derive macro depends on the `libp2p` crate to be in scope. This prevents standalone usage which forces us to depend on `libp2p` in all our tests where we want to derive a `NetworkBehaviour`.

This PR introduces a `prelude` option that - by default - points to `libp2p::swarm::derive_prelude`, a new module added to `libp2p_swarm`. With this config option, users of `libp2p_swarm` can now refer to the macro without depending on `libp2p`, breaking the circular dependency in our workspace. For consistency with the ecosystem, the macro is now also re-exported by `libp2p_swarm` instead of `libp2p` at the same position as the trait that it implements.

Lastly, we introduce an off-by-default `macros` feature flag that shrinks the dependency tree for users that don't need the derive macro.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants