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

Ensure assorted non-engine crates are formatted in CI #10768

Open
alice-i-cecile opened this issue Nov 27, 2023 · 6 comments
Open

Ensure assorted non-engine crates are formatted in CI #10768

alice-i-cecile opened this issue Nov 27, 2023 · 6 comments
Labels
A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes

Comments

@alice-i-cecile
Copy link
Member

          Yep. This is what we have to do, otherwise someone else is gonna run into this eventually.

Additionally, from the Bevy side, this should be checked by CI.

Originally posted by @rlidwka in #10758 (review)

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-Build-System Related to build systems or continuous integration labels Nov 27, 2023
@matiqo15
Copy link
Member

The issue here lies in that benches are not checked by cargo fmt --all -- --check. I've found two ways to solve it:

  1. We can add "benches" to members of workspace in Cargo.toml. However, this could add additional overhead - especially as "benches" are currently explicitly excluded from members.
  2. We could add flag in tools/ci/src/main.rs. This would then be: cmd!(sh, "cargo fmt --all --manifest-path ../../../benches/Cargo.toml -- --check"). While not best, it seems to be the only solution I've found if we don't want to add benches to workspace.

@alice-i-cecile
Copy link
Member Author

I think we should start with solution 2 for now.

@mockersf
Copy link
Member

mockersf commented Nov 29, 2023

As you said, the issue is that benches (and compile fail crates) are excluded from the workspace:

bevy/Cargo.toml

Lines 15 to 21 in 4221f7e

[workspace]
exclude = [
"benches",
"crates/bevy_ecs_compile_fail_tests",
"crates/bevy_macros_compile_fail_tests",
"crates/bevy_reflect_compile_fail_tests",
]

I don't remember why, but if we want the benches to be checked in CI (format, clippy, ...) they should be in the workspace.

@BD103 BD103 added C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Jul 18, 2024
@BD103
Copy link
Member

BD103 commented Feb 11, 2025

I fixed part of this issue in #16858 when I added benchmarks and compile_fail tests to the workspace. In #17330, however, new integration tests were added that are excluded from the workspace. @raldone01 is there a particular reason why you chose to exclude them?

@raldone01
Copy link
Contributor

raldone01 commented Feb 11, 2025

The reason the integration tests were excluded from the workspace was because their whole purpose is to be standalone crates that depend on bevy.

I think there was an issue with them also being part of the bevy workspace.

I did not want to add more complexity/magic to the integration tests by copy Cargo tomls and so on. The integration tests work like regular crates when you cd into them.

@BD103
Copy link
Member

BD103 commented Feb 12, 2025

Makes sense! I suppose being part of the workspace could affect their dependency resolution / inappropriately reuse build artifacts. Since it seems ideal to keep integration tests separate from the main workspace, I'll just modify the issue title to mention them.

Edit: Turns out I cannot edit titles. To anyone in the future planning on tackling this, your goal is to ensure the integration tests are checked with rustfmt in CI without adding them to the workspace. Benchmarks and tool crates are already in the workspace at this point, so don't worry about them! :)

@alice-i-cecile alice-i-cecile changed the title Validate that benches and tools crates are formatted in CI Ensure assorted non-engine crates are formatted in CI Feb 12, 2025
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 C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes
Projects
None yet
Development

No branches or pull requests

5 participants