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

Add benchmarks and compile_fail tests back to workspace #16858

Merged
merged 13 commits into from
Dec 21, 2024

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Dec 17, 2024

Objective

Solution

  • Add benchmarks and compile_fail tests to the Cargo workspace.
  • Fix any leftover formatting issues and documentation.

Testing

  • I think CI should catch most things!

Questions

Outdated issue I was having with function reflection being optional

The reflection_types example is failing in Rust-Analyzer for me, but not a normal check.

error[E0004]: non-exhaustive patterns: `ReflectRef::Function(_)` not covered
   --> examples/reflection/reflection_types.rs:81:11
    |
81  |     match value.reflect_ref() {
    |           ^^^^^^^^^^^^^^^^^^^ pattern `ReflectRef::Function(_)` not covered
    |
note: `ReflectRef<'_>` defined here
   --> /Users/bdeep/dev/bevy/bevy/crates/bevy_reflect/src/kind.rs:178:1
    |
178 | pub enum ReflectRef<'a> {
    | ^^^^^^^^^^^^^^^^^^^^^^^
...
188 |     Function(&'a dyn Function),
    |     -------- not covered
    = note: the matched value is of type `ReflectRef<'_>`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
126 ~         ReflectRef::Opaque(_) => {},
127 +         ReflectRef::Function(_) => todo!()
    |

I think it is because the following line is feature-gated:

// `Function` is a special trait that can be manually implemented (instead of deriving Reflect).
// This exposes "function" operations on your type, such as calling it with arguments.
// This trait is automatically implemented for types like DynamicFunction.
// This variant only exists if the `reflect_functions` feature is enabled.
#[cfg(feature = "reflect_functions")]
ReflectRef::Function(_) => {}

My theory for why this is happening is because the benchmarks enabled bevy_reflect's function feature, which gets merged with the rest of the features when RA checks the workspace, but the #[cfg(...)] gate in the example isn't detecting it:

bevy_reflect = { path = "../crates/bevy_reflect", features = ["functions"] }

Any thoughts on how to fix this? It's not blocking, since the example still compiles as normal, but it's just RA and the command cargo check --workspace --all-targets appears to fail.

Custom profiles must be in the workspace root, so it was raising a warning. I opted to delete it entirely, since I'm not sure LTO is a good idea for benchmarks anyways.
@BD103 BD103 added C-Code-Quality A section of code that is hard to understand or change A-Cross-Cutting Impacts the entire engine S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 17, 2024
@BD103 BD103 marked this pull request as ready for review December 17, 2024 16:20
@BD103 BD103 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 17, 2024
@alice-i-cecile
Copy link
Member

I take it the RA bug you mentioned is now fixed?

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Yay! And good comments too.

@BD103
Copy link
Member Author

BD103 commented Dec 17, 2024

I take it the RA bug you mentioned is now fixed?

No, not yet unfortunately, and it's also surfaced in the CI failure. There are a few solutions, but I don't like any of them:

  1. Conditionally-compile function reflection benchmarks, no longer enable the feature by default in benches/Cargo.toml.
  2. Enable function reflection by default and assume it's available in the example.
  3. Don't add benches to the Cargo workspace.
  4. Change how we detect function reflection in the example. I have no idea how we'd do this, but it would be preferable.

I think this is an underlying symptom of a larger problem: feature flags are difficult to use correctly and consistently. We don't have a system for defining how and when they should be made, and it's difficult to detect when they're enabled in our examples because we tunnel features through bevy and bevy_internal.

@BD103 BD103 added S-Needs-Investigation This issue requires detective work to figure out what's going wrong and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 17, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Help The author needs help finishing this PR. and removed S-Needs-Investigation This issue requires detective work to figure out what's going wrong labels Dec 17, 2024
@BD103 BD103 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Help The author needs help finishing this PR. labels Dec 19, 2024
@BD103

This comment was marked as outdated.

@alice-i-cecile alice-i-cecile added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Dec 19, 2024
@BD103 BD103 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 21, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 21, 2024
Merged via the queue into bevyengine:main with commit 2027700 Dec 21, 2024
34 checks passed
@BD103 BD103 deleted the add-crates-back-to-workspace branch December 22, 2024 21:17
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
…#16858)

# Objective

- Our benchmarks and `compile_fail` tests lag behind the rest of the
engine because they are not in the Cargo workspace, so not checked by
CI.
- Fixes bevyengine#16801, please see it for further context!

## Solution

- Add benchmarks and `compile_fail` tests to the Cargo workspace.
- Fix any leftover formatting issues and documentation.

## Testing

- I think CI should catch most things!

## Questions

<details>
<summary>Outdated issue I was having with function reflection being
optional</summary>

The `reflection_types` example is failing in Rust-Analyzer for me, but
not a normal check.

```rust
error[E0004]: non-exhaustive patterns: `ReflectRef::Function(_)` not covered
   --> examples/reflection/reflection_types.rs:81:11
    |
81  |     match value.reflect_ref() {
    |           ^^^^^^^^^^^^^^^^^^^ pattern `ReflectRef::Function(_)` not covered
    |
note: `ReflectRef<'_>` defined here
   --> /Users/bdeep/dev/bevy/bevy/crates/bevy_reflect/src/kind.rs:178:1
    |
178 | pub enum ReflectRef<'a> {
    | ^^^^^^^^^^^^^^^^^^^^^^^
...
188 |     Function(&'a dyn Function),
    |     -------- not covered
    = note: the matched value is of type `ReflectRef<'_>`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
126 ~         ReflectRef::Opaque(_) => {},
127 +         ReflectRef::Function(_) => todo!()
    |
```

I think it is because the following line is feature-gated:


https://github.com/bevyengine/bevy/blob/cc0f6a8db43581755bd84302072c7b97ea51bc0f/examples/reflection/reflection_types.rs#L117-L122

My theory for why this is happening is because the benchmarks enabled
`bevy_reflect`'s `function` feature, which gets merged with the rest of
the features when RA checks the workspace, but the `#[cfg(...)]` gate in
the example isn't detecting it:


https://github.com/bevyengine/bevy/blob/cc0f6a8db43581755bd84302072c7b97ea51bc0f/benches/Cargo.toml#L19

Any thoughts on how to fix this? It's not blocking, since the example
still compiles as normal, but it's just RA and the command `cargo check
--workspace --all-targets` appears to fail.

</summary>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add benchmarks and compile fail tests back to workspace
3 participants