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

Support and test compiling with -Z minimal-versions #9593

Open
SkiFire13 opened this issue Aug 26, 2023 · 5 comments
Open

Support and test compiling with -Z minimal-versions #9593

SkiFire13 opened this issue Aug 26, 2023 · 5 comments
Labels
A-Build-System Related to build systems or continuous integration A-Meta About the project itself C-Bug An unexpected or incorrect behavior

Comments

@SkiFire13
Copy link
Contributor

SkiFire13 commented Aug 26, 2023

-Z minimal-versions is an unstable cargo flag to generate a Cargo.lock with the minimal versions possible, rather than the latest ones. This helps finding cases where a crate depends on features of a dependency which were released in a later semver-compatible version than the one reported in the Cargo.toml.

I tried running cargo minimal-versions (which automates a bunch of work you have to do to correctly use -Z minimal-versions) and the result was not that good, although most issues were due to dependencies not respecing minimal versions themself.

I found the following issues (though there could be more hidden, see the final note):

  • bevy_ecs/bevy_utils should depend on tracing 0.1.36, since that version implemented Value for String which is used in bevy_ecs (although it uses the tracing macros re-exported from bevy_utils);
  • bevy_reflect should depend on lock_api 0.4.7, since that version introduced the const new function which bevy_reflect uses (also see Bevy-reflect: cannot call non-const fn in constant functions #9374, this could be considered a bug in parking_lot, since lock_api is a transitive dependency through it);
  • bevy_reflect should depend on thiserror 1.0.25, since that version introduced the ability to derive Error on non-'static types which is used by bevy_reflect;
  • bevy_pbr should depend on bitflags 2.3.1, since that version fixes the use of Self:: in the macro, which is used by bevy_pbr;
  • bevy_pbr should depend on meshopt 0.2.1 since that version introduced meshopt::ffi::{meshopt_optimizeMeshlet, simplify_scale} which are used in bevy_pbr (see also Use correct minimal version of meshopt #13551);
  • bevy_render should depend on image 0.24.3, since that version introduced the exr feature flag (before it was called openexr), which is used by bevy_render (see Fix bevy_render's image dependency version #14505);
  • bevy_text should depend on glam 0.24.1, since that version introduced the Vec2::INFINITY associated constant (should be fixed by Fix erronenous glam version #9653);
  • bevy_render should depend on image 0.25.2 since that version introduced image::ImageReader used by bevy_render, instead it only depends on image 0.25
  • tools/example-showcase should depend on pbr 1.1.1, because pbr 1.1.0 had a bug with imports on Windows;
  • bytemuck used to declare only a dependency on bytemuck_derive 1 while it actually uses features of bytemuck_derive 1.1 and later. This was fixed on bytemuck 1.12, but bevy crates depend only on older versions of bytemuck (bevy_pbr depends on bytemuck 1, bevy_ui, bevy_core, bevy_sprite and bevy_render on bytemuck 1.5; also, the bevy crate depends on bytemuck 1.7 but only as a dev-dependency);
  • some crate depend transitively on winapi 0.2.5, but it doesn't compile. winapi 0.2.7 worked though;
  • gpu_allocator declares a dependency on backtrace 0.3, but it uses features of backtrace 0.3.3 and later. I would consider this a bug of gpu_allocator since the use is internal. This hasn't been fixed yet, though I submitted a PR (Fix minimal versions Traverse-Research/gpu-allocator#174).

I decided to report the last three issues because, while they are not strictly bevy's fault, they impact building bevy with minimal versions.

Note that the analysis must consider the whole workspace together (crates in tools included), and this may hide problems in the single crates due to newer dependencies specified by other crates (and this could also apply to dependencies of dependencies...). So for example if bevy_ecs declared a dependency on foo 1 but actually used features from foo 1.1, and bevy_reflect declared a dependency on foo 1.1, then no error would be raised while compiling bevy_ecs, even though it technically declared a wrong dependency. To find some of these kind of errors you would have to compile the single crates outside the workspace, which is a lot more work to do. This could be doable just for crates that can be used alone, like bevy_ecs and bevy_reflect though.

It would be nice if in the future this was automatically tested, to ensure that people don't get weird errors like in #9374, or recently one posted on Discord with tracing older than 0.1.36 (which seems to not happen on the master branch likely due to some transitive dependency).

@SkiFire13 SkiFire13 added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Aug 26, 2023
@alice-i-cecile alice-i-cecile added A-Build-System Related to build systems or continuous integration A-Meta About the project itself and removed S-Needs-Triage This issue needs to be labelled labels Aug 26, 2023
@hymm
Copy link
Contributor

hymm commented Aug 27, 2023

I think the lock_api issue is at least partially why cargo run -p ci doesn't work on my machine. It's picking up 0.4.6 when I run that command, whereas it picks up 0.4.10 when I run cargo test or cargo run --example directly in my console.

@IceSentry
Copy link
Contributor

I just got hit by the bevy_text depending on 0.24 instead of 0.24.1 issue. I guess until we have CI to check this a PR that fixes all the issues you mentioned would probably be good.

@rlidwka
Copy link
Contributor

rlidwka commented Aug 31, 2023

looks like another dependency is found here: #9653

@hymm
Copy link
Contributor

hymm commented Sep 17, 2023

I tried adding lock_api = 0.4.7 to bevy_reflect, but it doesn't seem to work. I get a conflicting version error, when I try to run cargo run -p ci

Updating crates.io index
error: failed to select a version for `lock_api`.
    ... required by package `bevy_reflect v0.12.0-dev (E:\Github\bevy\crates\bevy_reflect)`
    ... which satisfies path dependency `bevy_reflect` (locked to 0.12.0-dev) of package `bevy_ecs v0.12.0-dev (E:\Github\bevy\crates\bevy_ecs)`
    ... which satisfies path dependency `bevy_ecs` (locked to 0.12.0-dev) of package `bevy_ecs_compile_fail_tests v0.1.0 (E:\Github\bevy\crates\bevy_ecs_compile_fail_tests)`
versions that meet the requirements `^0.4.7` are: 0.4.10, 0.4.9, 0.4.8, 0.4.7

all possible versions conflict with previously selected packages.

  previously selected package `lock_api v0.4.6`
    ... which satisfies dependency `lock_api = "^0.4.6"` (locked to 0.4.6) of package `parking_lot v0.12.1`
    ... which satisfies dependency `parking_lot = "^0.12.1"` (locked to 0.12.1) of package `bevy_reflect v0.12.0-dev (E:\Github\bevy\crates\bevy_reflect)`
    ... which satisfies path dependency `bevy_reflect` (locked to 0.12.0-dev) of package `bevy_ecs v0.12.0-dev (E:\Github\bevy\crates\bevy_ecs)`
    ... which satisfies path dependency `bevy_ecs` (locked to 0.12.0-dev) of package `bevy_ecs_compile_fail_tests v0.1.0 (E:\Github\bevy\crates\bevy_ecs_compile_fail_tests)`

failed to select a version for `lock_api` which could resolve this conflict
thread 'main' panicked at 'Compiler errors of the ECS compile fail tests seem to be different than expected! Check locally and compare rust versions.: command exited with non-zero code `cargo test --target-dir ../../target`: 101', tools\ci\src\main.rs:99:18

@SkiFire13
Copy link
Contributor Author

I tried adding lock_api = 0.4.7 to bevy_reflect, but it doesn't seem to work. I get a conflicting version error, when I try to run cargo run -p ci

I can reproduce this if I manually change the Cargo.lock file to use lock_api version 0.4.6, however there seems to be no good reason why it should be like this. Running cargo update inside crates/bevy_ecs_compile_fail_tests fixes the issue, but this shouldn't be necessary.

On the sidenote, does anyone know why the bevy_*_compile_fail_tests are excluded from the workspace? This prevents them from sharing the same Cargo.lock file, which might be a problem in this case.

@BD103 BD103 moved this to Idea in BD103's Bevy Work Jul 18, 2024
@BD103 BD103 moved this from Idea to Backburner in BD103's Bevy Work Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration A-Meta About the project itself C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

No branches or pull requests

5 participants