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

Create cfg aliases for bevy_app #18170

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

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();

@@ -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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants