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

cargo-workspace plugin cannot handle dependency cycles in dev-dependencies #1662

Open
thomaseizinger opened this issue Sep 28, 2022 · 3 comments · May be fixed by #1783
Open

cargo-workspace plugin cannot handle dependency cycles in dev-dependencies #1662

thomaseizinger opened this issue Sep 28, 2022 · 3 comments · May be fixed by #1783
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@thomaseizinger
Copy link

Thanks for stopping by to let us know something could be better!

PLEASE READ: If you have a support contract with Google, please create an issue in the support console instead of filing on GitHub. This will ensure a timely response.

  1. Is this a client library issue or a product issue?
    This is the client library for . We will only be able to assist with issues that pertain to the behaviors of this library. If the issue you're experiencing is due to the behavior of the product itself, please visit the Support page to reach the most relevant engineers.

It is a library issue.

  1. Did someone already solve this?
  1. Do you have a support contract?
    Please create an issue in the support console to ensure a timely response.

If the support paths suggested above still do not result in a resolution, please provide the following details.

Environment details

  • OS: Manjaro Linux
  • Node.js version: 18.8.0
  • npm version: 8.19.1
  • release-please version: 14.7.2

Steps to reproduce

  1. Have a cargo workspace that has dependency cycles with dev dependencies.
  2. Try to use the cargo-workspace plugin of release-please.

Cargo supports dependency cycles in the form of A -dep-> B -dev-dep-> A. In other words, I can have a crate A that depends on B but B can depend on A again in its dev-dependencies. release-please doesn't seem to allow this which I think is an unnecessary restriction. Version bumping of dev-dependencies should not impact the version of a package so perhaps dev-dependencies should just be ignored here: https://github.com/googleapis/release-please/blob/main/src/updaters/rust/common.ts#L71

Making sure to follow these steps will guarantee the quickest resolution possible.

Thanks!

@thomaseizinger thomaseizinger added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Sep 28, 2022
@chingor13 chingor13 self-assigned this Sep 30, 2022
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this issue 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
@chingor13
Copy link
Contributor

Sorry, for the super late response to this one.

It looks like there's 2 issues:

  1. release-please throws an error on circular dependencies which could be valid
  2. you'd like an option for a dev-dependency version bump not forcing a patch bump

@thomaseizinger
Copy link
Author

Sorry, for the super late response to this one.

It looks like there's 2 issues:

1. release-please throws an error on circular dependencies which could be valid

2. you'd like an option for a dev-dependency version bump not forcing a patch bump

Correct. We've resolved (1) on our end by getting rid of circular dependencies. That had other positive impacts.

I think (2) is still an issue. Bumping a dev-dependency should not warrant a version bump because it is not user facing.

@EqualMa
Copy link

EqualMa commented Feb 6, 2023

I found a workaround by renaming dependencies:

[dev-dependencies]
my-lib-dev = { path = "../my-lib", package = "my-lib" }

Then you can extern crate my_lib_dev as my_lib so that you can use ::my_lib other than ::my_lib_dev.

The cargo-workspace plugin is not smart enough to find out my-lib-dev is actually my-lib. Thus, cycle dependencies won't be reported.

@SurferJeffAtGoogle SurferJeffAtGoogle added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels Mar 15, 2023
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this issue 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
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants