Skip to content

Create cfg aliases for bevy_app #18170

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Create cfg aliases for bevy_app #18170

wants to merge 2 commits into from

Conversation

hukasu
Copy link
Contributor

@hukasu hukasu commented Mar 6, 2025

Objective

Implements partially feature requested in #1615

Solution

Create cfg aliases for some of the cfg used in bevy_app

Testing

Ran cargo run -p ci but needs more testing

Showcase

Before

#[cfg(all(any(unix, windows), feature = "std"))]
pub use terminal_ctrl_c_handler;
// ...
#[cfg(all(not(target_arch = "wasm32"), feature = "bevy_tasks"))]
bevy_tasks::tick_global_task_pools_on_main_thread();

After

#[cfg(std_windows_or_unix)]
pub use terminal_ctrl_c_handler::*;
// ...
#[cfg(can_run_tasks)]
bevy_tasks::tick_global_task_pools_on_main_thread();

bushrat011899
bushrat011899 reviewed Mar 6, 2025
View reviewed changes
crates/bevy_app/build.rs
@@ -0,0 +1,44 @@
//! Build script containing cfg aliases
Copy link
Contributor

Choose a reason for hiding this comment

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

The inclusion of a build.rs is definitely going to be the controversial component of this PR. Between security and build time concerns, I see this single-handedly being a show-stopper.

Personally, I'm not against it, since we have many dependencies that use cfg_aliases already (which is effectively what you're doing here by hand). But I think this will be very hard to land.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, cfg_aliases is already being used? i wanted to decrease the contentiousness by a little by not adding a new dependency, but if this is already present, maybe there won't be pushback on that point

Copy link
Contributor

Choose a reason for hiding this comment

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

It's already brought in as a build-dependency by winit and wgpu, so I wouldn't say it's a controversial inclusion in of itself. It's purely the use of a build.rs that I see as being controversial.

@bushrat011899 bushrat011899 added C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 6, 2025
@alice-i-cecile
Copy link
Member

build.rs is a complete non-starter for me. That way lies madness!

@alice-i-cecile alice-i-cecile added S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 6, 2025
@hukasu hukasu mentioned this pull request Apr 12, 2025
Merged
@NthTensor
Copy link
Contributor

Closing, the alternative (#18822) has been blessed by cart and seems on track to merge.

@NthTensor NthTensor closed this Apr 27, 2025
@hukasu hukasu deleted the cfg-aliases branch April 27, 2025 14:16
github-merge-queue bot pushed a commit that referenced this pull request May 6, 2025
@bushrat011899 @BD103
Create bevy_platform::cfg for viral feature management (#18822)
aadd3a3 
# Objective

- Acts on certain elements of #18799
- Closes #1615
- New baseline for #18170

## Solution

- Created a new `cfg` module in `bevy_platform` which contains two
macros to aid in working with features like `web`, `std`, and `alloc`.
- `switch` is a stable implementation of
[`cfg_match`](https://doc.rust-lang.org/std/macro.cfg_match.html), which
itself is a `core` alternative to [`cfg_if`](https://docs.rs/cfg-if).
- `define_alias` is a `build.rs`-free alternative to
[`cfg_aliases`](https://docs.rs/cfg_aliases) with the ability to share
feature information between crates.
- Switched to these macros within `bevy_platform` to demonstrate usage. 

## Testing

- CI

---

## Showcase

Consider the typical `std` feature as an example of a "virality". With
just `bevy_platform`, `bevy_utils`, and `bevy_ecs`, we have 3 crates in
a chain where activating `std` in any of them should really activate it
everywhere. The status-quo for this is for each crate to define its own
`std` feature, and ensure it includes the `std` feature of every
dependency in that feature. For crates which don't even interact with
`std` directly, this can be quite cumbersome. Especially considering
that Bevy has a fundamental crate, `bevy_platform`, which is a
dependency for effectively every crate.

Instead, we can use `define_alias` to create a macro which will
conditionally compile code if and only if the specified configuration
condition is met _in the defining crate_.

```rust
// In `bevy_platform`

define_alias! {
    #[cfg(feature = "std")] => {
        /// Indicates the `std` crate is available and can be used.
        std
    }
    #[cfg(all(target_arch = "wasm32", feature = "web"))] => {
        /// Indicates that this target has access to browser APIs.
        web
    }
}
```

The above `web` and `std` macros will either no-op the provided code if
the conditions are not met, or pass it unmodified if it is met. Since it
is evaluated in the context of the defining crate, `bevy_platform/std`
can be used to conditionally compile code in `bevy_utils` and `bevy_ecs`
_without_ those crates including their own `std` features.

```rust
// In `bevy_utils`
use bevy_platform::cfg;

// If `bevy_platform` has `std`, then we can too!
cfg::std! {
    extern crate std;
}
```

To aid in more complex configurations, `switch` is provided to provide a
`cfg_if` alternative that is compatible with `define_alias`:

```rust
use bevy_platform::cfg;

cfg::switch! {
    #[cfg(feature = "foo")] => { /* use the foo API */ }
    cfg::web => { /* use browser API */ }
    cfg::std => { /* use std */ }
    _ => { /* use a fallback implementation */ }
}
```

This paradigm would allow Bevy's sub-crates to avoid re-exporting viral
features, and also enable functionality in response to availability in
their dependencies, rather than from explicit features (bottom-up
instead of top-down). I imagine that a "full rollout" of this paradigm
would remove most viral features from Bevy's crates, leaving only
`bevy_platform`, `bevy_internal`, and `bevy` (since `bevy`/`_internal`
are explicitly re-exports of all of Bevy's crates).

This bottom-up approach may be useful in other areas of Bevy's features
too. For example, `bevy_core_pipeline/tonemapping_luts` requires:
- bevy_render/ktx2
- bevy_image/ktx2
- bevy_image/zstd

If `define_alias` was used in `bevy_image`, `bevy_render` would not need
to re-export the `ktx2` feature, and `bevy_core_pipeline` could directly
probe `bevy_image` for the status of `ktx2` and `zstd` features to
determine if it should compile the `tonemapping_luts` functionality,
rather than having an explicitly feature. Of course, an explicit feature
is still important for _features_, so this may not be the best example,
but it highlights that with this paradigm crates can reactively provide
functionality, rather than needing to proactively declare feature
combinations up-front and hope the user enables them.

---------

Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bushrat011899 bushrat011899 bushrat011899 left review comments

Assignees
No one assigned
Labels
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 S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hukasu @alice-i-cecile @NthTensor @bushrat011899