[Merged by Bors] - #4231: panic when App::run() is called from Plugin::build() #4241

Vrixyz · 2022-03-17T13:54:34Z

Objective

Solution

This PR implements the solution suggested by @bjorn3 : Use an internal property within App to detect App::run() calls from Plugin::build().

Changelog

panic when App::run() is called from Plugin::build()

crates/bevy_app/src/app.rs

mockersf · 2022-03-17T23:16:31Z

If this is wanted, it should be done for add_plugins too.

Buuuut... I don't think this is wanted. Why would we block a plugin from calling run? Should a RunPlugin that finish setting up my app then run it be impossible?

MrGVSV · 2022-03-18T00:20:54Z

Should a RunPlugin that finish setting up my app then run it be impossible?

Maybe not but in my opinion it's probably something we want users to do. It hides the run call (especially if the plugin has nested plugins). And it might lead to confusion if a user does it (or a crate or a team member) without fully understanding what they're doing.

I just find it to be a potential footgun. But that's just how I feel about it haha

Vrixyz · 2022-03-18T10:22:33Z

While I agree that panic is extreme, I believe allowing Plugins::build() to alter with app execution dramatically such as calling run() is something we want to at least discourage, or even forbid completely.

#1255 refers to order independence for plugins, which comfort this stance.

That said, I'm ok with displaying that as a warning if it's judged too extreme.

Also, if we have real use cases to call App::run() from Plugin::build(), we might want to provide a way to silence that warning.

bjorn3 · 2022-03-18T10:31:58Z

Plugins should use .set_runner() instead of running the app themself IMO. Having an a plugin run the app immediately would result in any calls after the current .add_plugin to be ignored.

alice-i-cecile · 2022-05-16T19:39:03Z

Plugins should use .set_runner() instead of running the app themself IMO. Having an a plugin run the app immediately would result in any calls after the current .add_plugin to be ignored.

I agree quite strongly with @bjorn3 here.

crates/bevy_app/src/app.rs

alice-i-cecile

Tiny nit, then this LGTM.

crates/bevy_app/src/app.rs

mockersf · 2022-05-16T20:58:55Z

I still really don't like this change, but if this gets to be merged could you add a panic section to the doc comment of the run method?

alice-i-cecile · 2022-05-17T04:05:24Z

Agreed; I'm going to block on that suggestion.

cart

I'm also not in love with this pattern, but I see it largely as an indicator that we should consider a more structured / limited plugin api.

I'm ok with adopting this in the interest of preventing a pattern that we will likely disallow (statically / structurally) in the future.

If conflicts are resolved I'm down to merge this.

alice-i-cecile · 2022-12-21T01:59:49Z

@Vrixyz feel free to rebase yourself and I'll remove the Adopt-Me label :) But we took ages on this so I want to make sure you don't feel obliged to.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Vrixyz · 2022-12-22T12:27:24Z

The rebase was not so straightforward, I ended up in setting is_building_plugin within App::add_boxed_plugin which is called by App::add_plugin and PluginGroupBuilder::finish.

I have a small doubt about dynamically loaded plugins which would not trigger the ~~warning~~ panic, but as it's a more advanced feature I guess it's fine.

alice-i-cecile · 2022-12-22T19:42:00Z

Yep I'm totally fine with this not working for dynamic plugins.

alice-i-cecile · 2022-12-24T23:43:24Z

bors r+

@bjorn3

# Objective Fixes #4231. ## Solution This PR implements the solution suggested by @bjorn3 : Use an internal property within `App` to detect `App::run()` calls from `Plugin::build()`. --- ## Changelog - panic when App::run() is called from Plugin::build()

bors · 2022-12-25T00:02:34Z

Pull request successfully merged into main.

Build succeeded:

@bjorn3

…bevyengine#4241) # Objective Fixes bevyengine#4231. ## Solution This PR implements the solution suggested by @bjorn3 : Use an internal property within `App` to detect `App::run()` calls from `Plugin::build()`. --- ## Changelog - panic when App::run() is called from Plugin::build()

@bjorn3

…bevyengine#4241) # Objective Fixes bevyengine#4231. ## Solution This PR implements the solution suggested by @bjorn3 : Use an internal property within `App` to detect `App::run()` calls from `Plugin::build()`. --- ## Changelog - panic when App::run() is called from Plugin::build()

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 17, 2022

Vrixyz force-pushed the 4231-plugin-build-warn-app-run branch from cbb3fcc to a35b026 Compare March 17, 2022 15:35

MrGVSV reviewed Mar 17, 2022

View reviewed changes

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved

ghost added A-App Bevy apps and plugins C-Startup A crash that occurs when first attempting to run a Bevy app and removed S-Needs-Triage This issue needs to be labelled labels Mar 18, 2022

Vrixyz added a commit to Vrixyz/bevy that referenced this pull request Mar 18, 2022

bevyengine#4241: fix PR feedbacks

00ebeec

alice-i-cecile reviewed May 16, 2022

View reviewed changes

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes May 16, 2022

View reviewed changes

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved

alice-i-cecile approved these changes May 16, 2022

View reviewed changes

alice-i-cecile requested review from MrGVSV and bjorn3 May 16, 2022 20:14

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 16, 2022

bjorn3 approved these changes May 16, 2022

View reviewed changes

tim-blackbird approved these changes May 16, 2022

View reviewed changes

MrGVSV approved these changes May 16, 2022

View reviewed changes

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 17, 2022

Vrixyz added a commit to Vrixyz/bevy that referenced this pull request May 18, 2022

bevyengine#4241: fix PR feedbacks

6c8a6dc

Vrixyz force-pushed the 4231-plugin-build-warn-app-run branch 2 times, most recently from 38435ae to 971bdbe Compare May 18, 2022 07:44

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR labels May 18, 2022

bjorn3 mentioned this pull request Oct 2, 2022

Show warning/prevent calling app .run() in Plugin build method #6145

Closed

cart approved these changes Dec 21, 2022

View reviewed changes

alice-i-cecile added S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed X-Controversial There is active debate or serious implications around merging this PR labels Dec 21, 2022

bevyengine#4231: panic when App::run() is called from Plugin::build()

87ea20c

Vrixyz added a commit to Vrixyz/bevy that referenced this pull request Dec 21, 2022

bevyengine#4241: fix PR feedbacks

fd3ac47

Vrixyz force-pushed the 4231-plugin-build-warn-app-run branch from 971bdbe to fe8abaa Compare December 21, 2022 13:38

Vrixyz and others added 3 commits December 21, 2022 15:01

bevyengine#4241: fix PR feedbacks

6a8dcf0

Update crates/bevy_app/src/app.rs

956fdbb

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

add panic section to App::run()

aa4d904

Vrixyz force-pushed the 4231-plugin-build-warn-app-run branch from fe8abaa to aa4d904 Compare December 21, 2022 14:02

bevyengine#4231: add test

171ab5b

alice-i-cecile removed the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Dec 22, 2022

bors bot changed the title ~~#4231: panic when App::run() is called from Plugin::build()~~ [Merged by Bors] - #4231: panic when App::run() is called from Plugin::build() Dec 25, 2022

bors bot closed this Dec 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - #4231: panic when App::run() is called from Plugin::build() #4241

[Merged by Bors] - #4231: panic when App::run() is called from Plugin::build() #4241

Vrixyz commented Mar 17, 2022

mockersf commented Mar 17, 2022

MrGVSV commented Mar 18, 2022

Vrixyz commented Mar 18, 2022 •

edited

Loading

bjorn3 commented Mar 18, 2022

alice-i-cecile commented May 16, 2022 •

edited

Loading

alice-i-cecile left a comment

mockersf commented May 16, 2022 •

edited

Loading

alice-i-cecile commented May 17, 2022

cart left a comment •

edited

Loading

alice-i-cecile commented Dec 21, 2022

Vrixyz commented Dec 22, 2022 •

edited

Loading

alice-i-cecile commented Dec 22, 2022

alice-i-cecile commented Dec 24, 2022

bors bot commented Dec 25, 2022

[Merged by Bors] - #4231: panic when App::run() is called from Plugin::build() #4241

[Merged by Bors] - #4231: panic when App::run() is called from Plugin::build() #4241

Conversation

Vrixyz commented Mar 17, 2022

Objective

Solution

Changelog

mockersf commented Mar 17, 2022

MrGVSV commented Mar 18, 2022

Vrixyz commented Mar 18, 2022 • edited Loading

bjorn3 commented Mar 18, 2022

alice-i-cecile commented May 16, 2022 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

mockersf commented May 16, 2022 • edited Loading

alice-i-cecile commented May 17, 2022

cart left a comment • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Dec 21, 2022

Vrixyz commented Dec 22, 2022 • edited Loading

alice-i-cecile commented Dec 22, 2022

alice-i-cecile commented Dec 24, 2022

bors bot commented Dec 25, 2022

Vrixyz commented Mar 18, 2022 •

edited

Loading

alice-i-cecile commented May 16, 2022 •

edited

Loading

mockersf commented May 16, 2022 •

edited

Loading

cart left a comment •

edited

Loading

Vrixyz commented Dec 22, 2022 •

edited

Loading