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.

Sign up for GitHub

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

Jump to bottom

fix: Remove circular dependencies across workspace #3023

Merged
merged 58 commits into from
Dec 12, 2022
Merged

fix: Remove circular dependencies across workspace #3023

merged 58 commits into from
Dec 12, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 14, 2022 •
edited
Loading

Description

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

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

@thomaseizinger

This comment was marked as resolved.

Sign in to view
@thomaseizinger thomaseizinger mentioned this pull request Oct 16, 2022
Closed
4 tasks
@thomaseizinger
Move derive tests to libp2p-swarm
3114d50 
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.
@thomaseizinger
Introduce derive_prelude
d6625cb
@thomaseizinger
Extract parse_attribute_value_by_key
3eec1fc
@thomaseizinger
Introduce prelude configuration option for NetworkBehaviour macro
16a856c
@thomaseizinger
Move NetworkBehaviour macro export from libp2p to libp2p_swarm
bd77e1d 
This allows us to depend on the macro without depending on the entire
`libp2p` crate which causes circular dependencies in our tests.
@thomaseizinger
Rename tests to make it clear what they are
e97f150
@thomaseizinger
Sort feature alphabetically
1c8654e
@thomaseizinger
Feature-gate NetworkBehaviour custom derive
8a5edfa
@thomaseizinger
Don't depend on the meta crate for dev-dependencies
7d74261
@thomaseizinger
Deprecate development transports
9b70a61 
These create circular dependencies to the root libp2p crate in tests.
@thomaseizinger thomaseizinger changed the base branch from master to 3053-move-swarm-derive October 24, 2022 01:42
@thomaseizinger thomaseizinger mentioned this pull request Oct 24, 2022
Closed
4 tasks
@galargh
Copy link
Contributor

galargh commented Oct 24, 2022

This is a draft PR as the current build process is a bit of a mess and doesn't catch all problems. Basically what happens is that when we build with --all-features, all the features in the libp2p crate also get activated so all crates that depend on the libp2p for the derive macro can just happily import their stuff through libp2p:: which is then a circular dependency and invalidates build-caches.

That's interesting! Do you know how other projects which have a similar setup are performing builds?

@thomaseizinger
Copy link
Contributor Author

This is a draft PR as the current build process is a bit of a mess and doesn't catch all problems. Basically what happens is that when we build with --all-features, all the features in the libp2p crate also get activated so all crates that depend on the libp2p for the derive macro can just happily import their stuff through libp2p:: which is then a circular dependency and invalidates build-caches.

That's interesting! Do you know how other projects which have a similar setup are performing builds?

Sorry, that description is outdated as of the latest commits.

Nope, I haven't looked at any other projects yet.

@thomaseizinger thomaseizinger changed the base branch from 3053-move-swarm-derive to master November 13, 2022 02:11
@mergify
Copy link
Contributor

mergify bot commented Nov 25, 2022

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

@thomaseizinger
Copy link
Contributor Author

@mxinden Can you give this a re-approval? A few things changed since you last reviewed this! @jxs I wouldn't mind getting a review from you too!

I think this is a non-breaking change because I only touch tests and examples but let me know if you can spot anything either way!

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Dec 7, 2022 •
edited
Loading

@mxinden Can you give this a re-approval? A few things changed since you last reviewed this! @jxs I wouldn't mind getting a review from you too!

I think this is a non-breaking change because I only touch tests and examples but let me know if you can spot anything either way!

The motivation for landing this is (among other things) to make release-please work which - together with semantic commits - avoids the manual bumping of manifests and writing of changelogs.

mxinden
mxinden approved these changes Dec 8, 2022
View reviewed changes
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.

This looks good to me. Thanks especially for the developer experience improvement.

.github/workflows/ci.yml Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

CI error should be fixed now, please merge whenever you are ready @mxinden!

@mergify
Copy link
Contributor

mergify bot commented Dec 9, 2022

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

@mxinden mxinden mentioned this pull request Dec 9, 2022
Closed
@mxinden
Copy link
Member

mxinden commented Dec 9, 2022

WebRTC failure seems to be consistent (i.e. non-flaky as assumed in #3223). @thomaseizinger can you take a look?

@thomaseizinger
Copy link
Contributor Author

WebRTC failure seems to be consistent (i.e. non-flaky as assumed in #3223). @thomaseizinger can you take a look?

Can confirm, it is not a flake. The new "swarmless" test doesn't work for WebRTC because it is not polling the Transport whilst upgrading connections.

@thomaseizinger thomaseizinger linked an issue Dec 12, 2022 that may be closed by this pull request
webrtc/tests: data channel opening took longer than 10 seconds #3223
Closed
@thomaseizinger
Copy link
Contributor Author

Okay, test should pass now. The solution is a bit verbose. I think we should transition libp2p-muxer-test-harness into libp2p-core-test and use that for all transports and muxers so we can cut down on the duplication and complexity of testing transports.

@mergify mergify bot merged commit d7363a5 into master Dec 12, 2022
@thomaseizinger
Copy link
Contributor Author

It's been a long journey but I am very happy that this is merged 🎉

@thomaseizinger thomaseizinger mentioned this pull request Dec 20, 2022
Closed
2 tasks
@thomaseizinger thomaseizinger deleted the 2173-no-libp2p-dev-dependency branch February 24, 2023 14:48
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
@thomaseizinger
fix: Remove circular dependencies across workspace (libp2p#3023)
05dd63d 
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
Reviewers

@mxinden mxinden mxinden approved these changes

@jxs jxs jxs approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

webrtc/tests: data channel opening took longer than 10 seconds Avoid circular dependencies in workspace
4 participants
@thomaseizinger @galargh @mxinden @jxs