Make libp2p-core default features optional in all subcrates

[rubdos]

It's impossible to use libp2p as dependency without having libp2p-core with all its default features (example use case: I don't need/want secp256k1). The reason is that libp2p requires (a.o.) libp2p-swarm, which in turn has libp2p-core with default features.

This is probably a backwards incompatible change. I would suggest that libp2p forwards its own default feature to libp2p-*.

Additionally, we probably need to discuss which subcrates require what features from libp2p-core, and how to make sure that CI still covers the common situations in terms of feature flags.

[rubdos]

Ater the 888314e fixup, I got rid of secp :-)

[thomaseizinger]

Relevant discussion: #2146

Given lack of my own involvement on this front, I am more than happy to support merging a subset of what is discussed in that PR.

Generally, I think we should embrace opt-in more than opt-out. What do things look like if we just remove secp256k1 as a feature from libp2p-core?

[rubdos]

Relevant discussion: #2146

Ah, missed that. Seems like this PR then works towards #2173, and that default can soon be set to [] and a full feature flag can be added.

Given lack of my own involvement on this front, I am more than happy to support?merging a subset of what is discussed in that PR.

Generally, I think we should embrace opt-in more than opt-out. What do things look like if we just remove secp256k1 as a feature from libp2p-core?

Removing secp256k1 from core means that the default feature set is incompatible with things like IPFS, that might be not desirable.

Additional to full, you could consider a ipfs feature flag too, pulling in the standard ipfs-features... Just a suggestion though, not a thing that I'd take up.

[thomaseizinger]

Good idea, I'll add the proposal to issue!

[mxinden]

I am in favor of this step towards #2173.

This is probably a backwards incompatible change.

Yes. It would require a changelog entry.

I would suggest that libp2p forwards its own default feature to libp2p-*.

I am not quite sure I follow. Could you expand?

Additionally, we probably need to discuss which subcrates require what features from libp2p-core

Given that secp256k1 is the only feature on libp2p-core there is not much to discuss, no?

and how to make sure that CI still covers the common situations in terms of feature flags.

We have a CI step testing with all features enabled. Given that features should always be designed in an additive way, this should be good enough. Am I missing something?

[rubdos]

I am in favor of this step towards #2173.

Does this mean "I am in favor of this PR as a step towards #2173", or "I am in favor of discussing this approach in context of #2173"? Not sure how to read you here.

This is probably a backwards incompatible change.

Yes. It would require a changelog entry.

Ack, will add after resolving the previous point :-)

I would suggest that libp2p forwards its own default feature to libp2p-*.

I am not quite sure I follow. Could you expand?

If the meta crate has it's default enabled, it could foward that to all the subcrates:

[features]
default = [
    "yamux",
    # ...
    "libp2p-core/default",
    "libp2p-identify/default",
    # ...
]

But probably, the intention is that the meta crate works on a higher level than that, so feel free to dismiss that idea.

Additionally, we probably need to discuss which subcrates require what features from libp2p-core

Given that secp256k1 is the only feature on libp2p-core there is not much to discuss, no?

Yes, I went too abstract in my thinking too quickly :-)

and how to make sure that CI still covers the common situations in terms of feature flags.

We have a CI step testing with all features enabled. Given that features should always be designed in an additive way, this should be good enough. Am I missing something?

I would add some CI tests to check that certain features can be turned off, i.e., that they are in fact designed in an additive way.

[mxinden]

I am in favor of this step towards #2173.

Does this mean "I am in favor of this PR as a step towards #2173", or "I am in favor of discussing this approach in context of #2173"? Not sure how to read you here.

The former. In other words, I am in favor of both this pull request and #2173 and I think we don't have to block this pull request on #2173 and can thus already merge this pull request.

---

This is probably a backwards incompatible change.

Yes. It would require a changelog entry.

Ack, will add after resolving the previous point :-)

🙏 small caveat, for each crate touched in this pull request, one entry needs to be added to the corresponding CHANGELOG.md file. Sorry for the inconvenience :/

---

I would suggest that libp2p forwards its own default feature to libp2p-*.

I am not quite sure I follow. Could you expand?

If the meta crate has it's default enabled, it could foward that to all the subcrates:

[features]
default = [
    "yamux",
    # ...
    "libp2p-core/default",
    "libp2p-identify/default",
    # ...
]

But probably, the intention is that the meta crate works on a higher level than that, so feel free to dismiss that idea.

Ah, thanks for explaining. Yes, that would make sense to me, though I would guess we would want to forward the full feature with #2173 in mind. Either way, I think this should be a separate pull request.

---

and how to make sure that CI still covers the common situations in terms of feature flags.

We have a CI step testing with all features enabled. Given that features should always be designed in an additive way, this should be good enough. Am I missing something?

I would add some CI tests to check that certain features can be turned off, i.e., that they are in fact designed in an additive way.

Indeed, that would be nice to have. Would you have a test requiring said feature that is then expected to fail?

[rubdos]

I am in favor of this step towards #2173.

Does this mean "I am in favor of this PR as a step towards #2173", or "I am in favor of discussing this approach in context of #2173"? Not sure how to read you here.

The former. In other words, I am in favor of both this pull request and #2173 and I think we don't have to block this pull request on #2173 and can thus already merge this pull request.

Ack, I'll finish this up then.

This is probably a backwards incompatible change.

Yes. It would require a changelog entry.

Ack, will add after resolving the previous point :-)

pray small caveat, for each crate touched in this pull request, one entry needs to be added to the corresponding CHANGELOG.md file. Sorry for the inconvenience :/

Don't worry. Vim-fu will help.

I would suggest that libp2p forwards its own default feature to libp2p-*.

I am not quite sure I follow. Could you expand?

If the meta crate has it's default enabled, it could foward that to all the subcrates:

[features]
default = [
    "yamux",
    # ...
    "libp2p-core/default",
    "libp2p-identify/default",
    # ...
]

But probably, the intention is that the meta crate works on a higher level than that, so feel free to dismiss that idea.

Ah, thanks for explaining. Yes, that would make sense to me, though I would guess we would want to forward the full feature with #2173 in mind. Either way, I think this should be a separate pull request.

Makes sense.

and how to make sure that CI still covers the common situations in terms of feature flags.

We have a CI step testing with all features enabled. Given that features should always be designed in an additive way, this should be good enough. Am I missing something?

I would add some CI tests to check that certain features can be turned off, i.e., that they are in fact designed in an additive way.

Indeed, that would be nice to have. Would you have a test requiring said feature that is then expected to fail?

I think that just checking that everything compiles additively should be enough. Tests that require specific features will fail to compile anyway [citation needed]

[rubdos]

Hmm, on the other hand, failure testing would be nice to have; as an example: test that secp256p1 doesn't work when the feature is disabled.

Since you already test --no-default-features and --all-features, I'm not entirely sure what other feature sets to test (apart from maybe the default feature set?)

[mxinden]

Looks good to me. Thank @rubdos for tackling this.

I think this is a good step forward, allowing people to use libp2p without secp256k1. Will leave it open for a bit longer for others to comment as well.