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

libp2p_swarm_derive: Add crate parameter to configure libp2p crate name #3006

Closed

Conversation

stormshield-pj50
Copy link
Contributor

@stormshield-pj50 stormshield-pj50 commented Oct 10, 2022

Description

When importing libp2p with a use statement and a custom alias name, the code generated by the NetworkBehaviour derive macro does not use this alias and does not compile. This PR enhances the derive macro with a new parameter "crate" that can be used to specify the custom alias name.

Open Questions

None

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

@stormshield-pj50 stormshield-pj50 changed the title Allow libp2p_prefix parameter in NetworkBehaviour derive macro Allow crate parameter in NetworkBehaviour derive macro Oct 11, 2022
@thomaseizinger thomaseizinger changed the title Allow crate parameter in NetworkBehaviour derive macro libp2p_swarm_derive: Add crate parameter to configure libp2p crate name Oct 14, 2022
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

One suggestion. Otherwise looks good to me.

Comment on lines +50 to +65
let user_provided_libp2p_prefix = ast
.attrs
.iter()
.filter_map(get_meta_items)
.flatten()
.filter_map(|meta_item| {
if let syn::NestedMeta::Meta(syn::Meta::NameValue(ref m)) = meta_item {
if m.path.is_ident("crate") {
if let syn::Lit::Str(ref s) = m.lit {
return Some(syn::parse_str::<syn::TypePath>(&s.value()).unwrap());
}
}
}
None
})
.next();
Copy link
Member

Choose a reason for hiding this comment

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

This is mostly the same as the logic for out_event, right? What do you think of moving it into a function called in both places?

Comment on lines +136 to +151
#[test]
fn custom_crate() {
use libp2p as mylibp2p;
#[allow(dead_code)]
#[derive(NetworkBehaviour)]
#[behaviour(crate = "mylibp2p")]
struct Foo {
ping: ping::Behaviour,
identify: identify::Behaviour,
}

#[allow(dead_code)]
fn foo() {
require_net_behaviour::<Foo>();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This test also already passes with current master (if you remove line 141).
Correct me if I am wrong, but I believe the current ::libp2p prefix imports libp2p from the crate level, which means that aliases in a module should not be an issue? @mxinden?

I think it is only a problem if the crate is imported in the Cargo.toml with a different name.
serde on the other hand states for their crate attribute that:

This is normally only applicable when invoking re-exported Serde derives from a public macro in a different crate.

I am not against adding this parameter, but could we add a test that fails on current master and would be fixed with this? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, the issue is not clashing symbols but renaming the crate in your manifest.

To test that, we'd have to make a separate test crate with a renamed import.

This make me wonder, should we perhaps just require certain symbols to be present for the macro to work?

That would make #3023 easier to implement.

Or perhaps we could make a seperate "derive" prelude? Like libp2p::swarm_derive_prelude that includes all the required symbols. Then we can add a configuration knob here that allows you to change where that prelude is coming from.

Copy link
Contributor

Choose a reason for hiding this comment

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

This make me wonder, should we perhaps just require certain symbols to be present for the macro to work?

That would make #3023 easier to implement.

Or perhaps we could make a seperate "derive" prelude? Like libp2p::swarm_derive_prelude that includes all the required symbols. Then we can add a configuration knob here that allows you to change where that prelude is coming from.

Actually, don't worry about it, I already have a solution to that.

@thomaseizinger
Copy link
Contributor

If we end up shipping #3055, then this PR is obsolete as the prelude option allows you to achieve the same thing via:

#[behaviour(prelude = "mylibp2p::swarm::derive_prelude")

@thomaseizinger
Copy link
Contributor

Closing in favor of #3055 which is about to be merged.

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