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

initial duplicate bevy version lint #185

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

DaAlbrecht
Copy link
Contributor

@DaAlbrecht DaAlbrecht commented Nov 27, 2024

A first draft of a quick lint to check if multiple versions of bevy are defined. #15

todos:

  • create ui test
  • better error message, perhaps use cargo metadata to get information about the crate versions.
  • fix the span ( this should point to the crate causing the issue in the Cargo.toml)
error: Multiple versions of `bevy` found
  --> /Users/didi/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy-0.13.2/src/lib.rs:1:1
   |
1  | / #![allow(clippy::single_component_path_imports)]
2  | |
3  | | //! [![](https://bevyengine.org/assets/bevy_logo_d...
4  | | //!
...  |
51 | | #[allow(unused_imports)]
52 | | use bevy_dylib;
   | |_______________^
   |
   = note: `#[deny(bevy::duplicate_bevy_dependencies)]` on by default

Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

So far this looks good! I am interested in getting a span to Cargo.toml / Cargo.lock, though I'm unsure if the compiler will know that file exists. Ping me when this is ready for a final review!

bevy_lint/src/lints/duplicate_bevy_dependencies.rs Outdated Show resolved Hide resolved
@DaAlbrecht
Copy link
Contributor Author

example of the lint message

error: Mismatching versions of `bevy` found
  --> Cargo.toml:10:26
   |
10 | leafwing-input-manager = "0.13"
   |                          ^^^^^^
   |
help: Expected all crates to use `bevy` 0.14.2
  --> Cargo.toml:8:8
   |
8  | bevy = { version = "0.14.2", features = ["android_shared_stdcxx"] }
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@DaAlbrecht DaAlbrecht changed the title feat: initial duplicate bevy version lint WIP: initial duplicate bevy version lint Dec 10, 2024
@DaAlbrecht DaAlbrecht marked this pull request as draft December 10, 2024 23:59
@BD103
Copy link
Member

BD103 commented Dec 18, 2024

One function you may find useful is was_invoked_from_cargo(), which detects whether we're being run from Cargo or not. We can use this to skip Cargo lints when bevy_lint_driver is called directly!

@DaAlbrecht
Copy link
Contributor Author

One function you may find useful is was_invoked_from_cargo(), which detects whether we're being run from Cargo or not. We can use this to skip Cargo lints when bevy_lint_driver is called directly!

nice catch! Thanks will add this.

@DaAlbrecht DaAlbrecht changed the title WIP: initial duplicate bevy version lint initial duplicate bevy version lint Dec 24, 2024
@DaAlbrecht DaAlbrecht marked this pull request as ready for review December 29, 2024 21:35
@TimJentzsch TimJentzsch added the S-Needs-Review The PR needs to be reviewed before it can be merged label Jan 18, 2025
@TimJentzsch TimJentzsch added A-Linter Related to the linter and custom lints C-Feature Make something new possible labels Jan 18, 2025
@TimJentzsch
Copy link
Collaborator

@DaAlbrecht The tests are failing, because Bevy needs additional system packages to be installed on Linux.

I had the same issue in #222, so we can either merge that PR first and then merge main back in your branch or you can copy the CI-related changes here to fix this :)

bevy_lint/Cargo.toml Show resolved Hide resolved
bevy_lint/Cargo.toml Show resolved Hide resolved
Comment on lines +110 to +116
// Semver only supports checking if a given `VersionReq` matches a `Version` and not if two
// `VersionReq` can successfully resolve to one `Version`. Therefore we try to parse the
// `Version` from the `bevy` dependency in the `Cargo.toml` file. This only works if a
// single version of `bevy` is specified and not a range.
let Ok(target_version) = get_version_from_toml(bevy_cargo.as_ref()) else {
return;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, I'm not sure if this is the right approach here, as the version string in Cargo.toml really is a VersionReq instead of a Version.

In the cargo metadata output is a resolve object, which I believe essentially contains the dependency graph.
Do you think we could search that for bevy entries and then extract the versions from the package ID? Then we already know it's two different versions and don't need to compare them.

I guess it would make it a bit harder though to find where the duplicated bevy version is coming from (and which of both the "main" Bevy version is).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I think you are already doing something similar?
I'm not sure if I'm understanding the code correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, im sorry its a bit confusing here a "short" tldr (;

I think it would make sense for us to build our own helper function that checks if two VersionReqs can be resolved to one matching Version. But the semver crate does not support this right now. If I remember correctly my thought process was the following:

  • Users using bevy to build a game likely specify a specific version of bevy rather than a range.
  • Library authors are more likely to specify a version range than a specific version of bevy

So this allows to easily check with dependency requires a version of bevy that is not resolved to the same Version as the "main" bevy crate.

I guess it would make sense to do the minimal_lint instead of return here (when a version range is specified for the "main" bevy crate):

    let Ok(target_version) = get_version_from_toml(bevy_cargo.as_ref()) else {
        return;
    };

The minimal_lint does check if there are multiple entries in the resolved dependency graph that contain different bevy versions and if so just lints a warning but does not provide a span since I don't know which versions match the "main" bevy crate without resolving the VersionReq's of said "main" bevy crate and its dependents.

This Cargo.toml:

[package]
name = "test_project"
version = "0.1.0"
edition = "2021"

[dependencies]
bevy = { version = "0.14.2" }
avian2d = "0.1.2"
leafwing-input-manager = "0.13"

Has these sections in the Cargo.lock

[[package]]
name = "bevy"
version = "0.13.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "65b9eadaacf8fe971331bc3f250f35c18bc9dace3f96b483062f38ac07e3a1b4"
dependencies = [
 "bevy_internal 0.13.2",
]

[[package]]
name = "bevy"
version = "0.14.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "043c9ad4b6fc4ca52d779873a8ca792a4e37842d07fce95363c9e17e36a1d8a0"
dependencies = [
 "bevy_internal 0.14.2",
]

So I didn't know how to find the "main" bevy version.

Future

I think if we add a helper function that checks if two VersionReqs can be resolved to one Version we can just remove the minimal_lint and use that instead. This helps us to always get a span and a help message and additionally, we do not need to search the entire dependency graph.

error: Mismatching versions of `bevy` found
--> Cargo.toml:11:26
|
11 | leafwing-input-manager = "0.13"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super cool to have the spans here!

Bonus would be if it told you which Bevy version the dependency used instead, but that'd just be the cherry on top, feel free to leave it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

o yes! that's a very cool idea and would be pretty easy to add

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to include that, what do you think?

@BD103 BD103 linked an issue Jan 28, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Related to the linter and custom lints C-Feature Make something new possible S-Needs-Review The PR needs to be reviewed before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect duplicate bevy dependencies
3 participants