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

*: Remove default features from all crates #2918

Merged
merged 31 commits into from
Sep 29, 2022
Merged

Conversation

thomaseizinger
Copy link
Contributor

Description

This patch-set removes our use of default features. We also introduce a new full feature that activates all features.

Links to any relevant issues

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

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.

Thanks for tackling this.

As mentioned elsewhere, I am in favor of this change.

Given that this is a large breaking change for users, is there anyone objecting to this change?

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -46,7 +46,7 @@ ring = { version = "0.16.9", features = ["alloc", "std"], default-features = fal
async-std = { version = "1.6.2", features = ["attributes"] }
base64 = "0.13.0"
criterion = "0.4"
libp2p = { path = "../", default-features = false, features = ["mplex", "noise"] }
libp2p = { path = "..", features = ["full"] }
Copy link
Member

Choose a reason for hiding this comment

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

Say I want to run the libp2p-core unit tests locally, wouldn't it be nicer to only compile the libp2p features that I need? Is this use-case worth the additional effort?

Copy link
Contributor Author

@thomaseizinger thomaseizinger Sep 22, 2022

Choose a reason for hiding this comment

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

It is a pain though to keep track of the features you need to activate :/

Plus, with all crates requesting different features, I actually think we have to compile things multiple times to satisfy each combination. If the convention is to enable full everywhere, we only need to compile every crate once to and the compiled artifact can be used for every crate's tests1.

I think the convenience of just saying "full" is worth it. Locally you have caches of everything so you will most likely just pay in linker time and if you modify your code between test executions, you gotta relink the test binary anyway so I am not sure it will matter in practice.

Can we try it out and if it negatively affects anything, figure out how to make it better?

Footnotes

  1. This is an unproven hypothesis.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for elaborating. Using full sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed my mind. Esp. working on libp2p-core is quite painful now because every change there invalidates libp2p-core artifact which causes a re-compile of every other crate.

Copy link
Member

Choose a reason for hiding this comment

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

👍 mind proposing a patch @thomaseizinger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I think the best way to resolve this is to actually stop the circular dependency. This will allow for proper caching of the build artifacts. It will also solve the issue I am currently facing with release-please (#2902).

Co-authored-by: Max Inden <[email protected]>
@thomaseizinger
Copy link
Contributor Author

Given that this is a large breaking change for users, is there anyone objecting to this change?

Is it that large a change? Activate a couple of feature flags and your back in. No functionality is changing!

@thomaseizinger
Copy link
Contributor Author

Given that this is a large breaking change for users, is there anyone objecting to this change?

Is it that large a change? Activate a couple of feature flags and your back in. No functionality is changing!

I remember tokio moving to this convention taking me a couple of h. I am not saying it is hard. Just saying it is tedious. Never the less I am in favor of this change.

@thomaseizinger do you want this to be included in v0.49.0 (#2931)?

That would be great, yeah!

@thomaseizinger thomaseizinger mentioned this pull request Sep 23, 2022
4 tasks
@dignifiedquire
Copy link
Member

I would be great to have a short section "How to upgrade" for the release, so folks now how to adjust their feature usage.

@thomaseizinger
Copy link
Contributor Author

I would be great to have a short section "How to upgrade" for the release, so folks now how to adjust their feature usage.

Is the first sentence in the changelog entry not good enough? https://github.com/libp2p/rust-libp2p/blob/2173-no-default-features/CHANGELOG.md#0490---unreleased

@dignifiedquire
Copy link
Member

Is the first sentence in the changelog entry not good enough?

I somehow missed that when looking through the diff. LGTM 👍

@mxinden
Copy link
Member

mxinden commented Sep 27, 2022

@thomaseizinger would you mind resolving the merge conflicts here? No rush, just a ping.

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger would you mind resolving the merge conflicts here? No rush, just a ping.

Sure! I just didn't realize there are any :/
I opened #2948.

@thomaseizinger
Copy link
Contributor Author

@mxinden Resolved.

@mxinden mxinden merged commit f6bb846 into master Sep 29, 2022
@thomaseizinger thomaseizinger deleted the 2173-no-default-features branch September 29, 2022 22:34
@thomaseizinger
Copy link
Contributor Author

Wooooo! This is so exciting!

elenaf9 added a commit to elenaf9/rust-libp2p that referenced this pull request Oct 2, 2022
elenaf9 added a commit to kpp/rust-libp2p that referenced this pull request Oct 3, 2022
@umgefahren
Copy link
Contributor

As long as #2950 is still open, this could result in a very confusing experience for a beginner. We should definitely fix that.

mergify bot pushed a commit that referenced this pull request Dec 12, 2022
Circular dependencies are problematic in several ways:

- They result in cognitive overhead for developers, in trying to figure out what depends on what.
- They present `cargo` with limits in what order the crates can be compiled in.
- They invalidate build caches unnecessarily thus forcing `cargo` to rebuild certain crates.
- They cause problems with tooling such as `release-please`.

To actually break the circular dependencies, this patch inlines the uses of `development_transport` in the examples and tests for all sub-crates. This is only meant to be a short-term fix until #3111 and #2888 are fixed.

To ensure we don't accidentally reintroduce this dependency, we add a basic CI that queries `cargo metadata` using `jq`.

Resolves #3053.
Fixes #3223.
Related: #2918 (comment)
Related: googleapis/release-please#1662
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
Remove default features. You need to enable required features
explicitly now. As a quick workaround, you may want to use the
new `full` feature which activates all features.
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
Circular dependencies are problematic in several ways:

- They result in cognitive overhead for developers, in trying to figure out what depends on what.
- They present `cargo` with limits in what order the crates can be compiled in.
- They invalidate build caches unnecessarily thus forcing `cargo` to rebuild certain crates.
- They cause problems with tooling such as `release-please`.

To actually break the circular dependencies, this patch inlines the uses of `development_transport` in the examples and tests for all sub-crates. This is only meant to be a short-term fix until libp2p#3111 and libp2p#2888 are fixed.

To ensure we don't accidentally reintroduce this dependency, we add a basic CI that queries `cargo metadata` using `jq`.

Resolves libp2p#3053.
Fixes libp2p#3223.
Related: libp2p#2918 (comment)
Related: googleapis/release-please#1662
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.

4 participants