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

Deprecate dynamic plugins #13080

Merged
merged 7 commits into from
May 20, 2024
Merged

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Apr 23, 2024

Objective

Solution

  • Deprecate all dynamic plugin items for Bevy 0.14, with plans to remove them for Bevy 0.15.

Discussion

One thing I want to make clear is that I'm not opposed to dynamic plugins in general. I think they can be handy, especially for DLC and modding, but I think the current system is the wrong approach. It's too much of a footgun for the meager benefit is provides.


Changelog

  • Deprecated the current dynamic plugin system.
    • Dynamic plugins will be removed in Bevy 0.15. For now you can continue using them by marking your code with #[allow(deprecated)].

Migration Guide

If possible, remove all usage of dynamic plugins.

// Old
#[derive(DynamicPlugin)]
pub struct MyPlugin;

App::new()
    .load_plugin("path/to/plugin")
    .run();

// New
pub struct MyPlugin;

App::new()
    .add_plugins(MyPlugin)
    .run();

If you are unable to do that, you may temporarily silence the deprecation warnings.

#[allow(deprecated)]

Please note that the current dynamic plugin system will be removed by the next major Bevy release, so you will have to migrate eventually. You may be interested in these safer alternatives:

@BD103 BD103 added A-App Bevy apps and plugins C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide P-Unsound A bug that results in undefined compiler behavior C-Needs-Release-Note Work that should be called out in the blog due to impact labels Apr 23, 2024
@BD103
Copy link
Member Author

BD103 commented Apr 23, 2024

I've added the "needs release notes" label, though I can be convinced otherwise. I don't know how large of an impact this change will make.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 23, 2024
@BD103 BD103 force-pushed the deprecate-dynamic-plugins branch from 852fc12 to 9b6ef6f Compare April 23, 2024 22:22
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Apr 23, 2024

Previously attempted in #3893

See #3893 (comment) for Cart's opinion at the time.

My position remains about the same as it was then:

  1. Improving this crate is a poor use of our organizational time currently
  2. There have been approximately 0 meaningful PRs to the crate ever
  3. It presents a misleading and unsafe rabbit hole for new users

Given the presence of testing showing indications of UB during reasonable use, I think now's as good a time as any to officially drop support.

@BD103 BD103 marked this pull request as ready for review April 23, 2024 22:31
@mockersf
Copy link
Member

maybe the deprecated comment could redirect to something like "go comment on this pr if you see this message" to encourage potential users to report back

@NthTensor
Copy link
Contributor

I'd also like to express my support for this change. Aside from the correctness issues, the current version also effectively blocks some advanced plugin build ordering/order-independence features.

@BD103
Copy link
Member Author

BD103 commented Apr 24, 2024

maybe the deprecated comment could redirect to something like "go comment on this pr if you see this message" to encourage potential users to report back

I added a deprecation notice in 1b97d99. Hopefully if the alternatives listed do not support someone's use-case, they will comment here. (It may help designing a new API!)

@alice-i-cecile 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 Apr 24, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Apr 24, 2024
Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

yeah, this makes sense. working with this crate requires a lot of finesse, and from my woes a few years 👀 ago, once you have the conceptual understanding needed to use this crate, its really not very much work to implement it from scratch yourself.

@nerdachse
Copy link

I am sorry, how again is this controversial? I haven't seen any comment indicating such.

@NthTensor
Copy link
Contributor

This was controversial the last time it was tried, which makes this sort of controversial-by-default.

@BD103
Copy link
Member Author

BD103 commented May 3, 2024

I am sorry, how again is this controversial? I haven't seen any comment indicating such.

@cart disagreed with this change last time it was attempted. (#3893 (comment)) No one has picked up dynamic plugins and tried to improve it since deprecating it was last attempted.

Furthermore, I think bevy_dynamic_plugin even existing in the main Bevy project is encouraging users to use it instead of making their own, better alternatives. As someone mentioned on Discord, Bevy has a reputation for reliability (partially due to being written in Rust). I would argue that dynamic plugins are quite unreliable, hurting this reputation. They are simple enough that if someone wanted to use them, they could just copy the code. I think they should do this, because it puts the responsibility on them and not us.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I relent. A bit bummed that we're removing useful baseline functionality, but I agree that using it soundly is hard and that exposes people to risk. I won't fight this anymore :)

@mockersf mockersf added this pull request to the merge queue May 20, 2024
Merged via the queue into bevyengine:main with commit 2940636 May 20, 2024
27 checks passed
@BD103 BD103 deleted the deprecate-dynamic-plugins branch May 20, 2024 20:23
salvadorcarvalhinho pushed a commit to salvadorcarvalhinho/bevy that referenced this pull request May 25, 2024
# Objective

- The current implementation for dynamic plugins is unsound. Please see
bevyengine#11969 for background and justification.
- Closes bevyengine#11969 and closes bevyengine#13073.

## Solution

- Deprecate all dynamic plugin items for Bevy 0.14, with plans to remove
them for Bevy 0.15.

## Discussion

One thing I want to make clear is that I'm not opposed to dynamic
plugins _in general_. I think they can be handy, especially for DLC and
modding, but I think the current system is the wrong approach. It's too
much of a footgun for the meager benefit is provides.

---

## Changelog

- Deprecated the current dynamic plugin system.
- Dynamic plugins will be removed in Bevy 0.15. For now you can continue
using them by marking your code with `#[allow(deprecated)]`.

## Migration Guide

If possible, remove all usage of dynamic plugins.

```rust
// Old
#[derive(DynamicPlugin)]
pub struct MyPlugin;

App::new()
    .load_plugin("path/to/plugin")
    .run();

// New
pub struct MyPlugin;

App::new()
    .add_plugins(MyPlugin)
    .run();
```

If you are unable to do that, you may temporarily silence the
deprecation warnings.

```rust
#[allow(deprecated)]
```

Please note that the current dynamic plugin system will be removed by
the next major Bevy release, so you will have to migrate eventually. You
may be interested in these safer alternatives:

- [Bevy Assets - Scripting]: Scripting and modding libraries for Bevy
- [Bevy Assets - Development tools]: Hot reloading and other development
functionality
- [`stabby`]: Stable Rust ABI

[Bevy Assets - Scripting]: https://bevyengine.org/assets/#scripting
[Bevy Assets - Development tools]:
https://bevyengine.org/assets/#development-tools
[`stabby`]: https://github.com/ZettaScaleLabs/stabby
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1321 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Needs-Release-Note Work that should be called out in the blog due to impact P-Unsound A bug that results in undefined compiler behavior 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
Projects
Status: Responded
7 participants