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 current bevy_dynamic_plugin implementation #11969

Closed
BD103 opened this issue Feb 19, 2024 · 3 comments · Fixed by #13080
Closed

Deprecate current bevy_dynamic_plugin implementation #11969

BD103 opened this issue Feb 19, 2024 · 3 comments · Fixed by #13080
Labels
A-App Bevy apps and plugins P-Unsound A bug that results in undefined compiler behavior X-Controversial There is active debate or serious implications around merging this PR

Comments

@BD103
Copy link
Member

BD103 commented Feb 19, 2024

What problem does this solve or what need does it fill?

bevy_dynamic_plugin is likely unsound, or at the very least so dangerous that it should not be an officially sanctioned crate. It currently works by deriving bevy_app::DynamicPlugin onto a unit struct that implements Plugin. The code can then be built as a cdylib and loaded with bevy_dynamic_plugin::dynamically_load_plugin.

There are a few potential footguns / problems with this approach:

  • It works by loading and executing foreign code, which is drastically unsafe.
    • Loading a library could run ctor code unconditionally, without calling any functions.
  • Plugins must be built with the exact same Bevy and Rust versions.
    • This prevents plugins from taking advantage of new optimizations in the compiler and important bug fixes in Bevy.
    • #[derive(DynamicPlugin)] creates _bevy_create_plugin, which returns &'static dyn Plugin. dyn references are fat pointers, where the metadata is a VTable. VTable layouts are not guaranteed.
  • Lots of Bevy uses generics and type-level code, especially in App.
    • I don't believe Rust provides any guarantees with how generics / types are generated, even across compiles with the same version. (This needs fact-checked.)
    • There's also a lot of TypeId usage. TypePath somewhat fixes this, but it's not used everywhere.

What solution would you like?

Mark dynamically_load_plugin and DynamicPluginExt::load_plugin as deprecated for Bevy 0.14. In Bevy 0.15, they will be removed and bevy_dynamic_plugin will be turned into an empty crate likely moved to bevy-crate-reservations.

I think the current implementation is just too unsafe to continue in the main Bevy library. As mentioned by Alice, it may be worth recommending dextrous_developer in the module or deprecation notes.

What alternative(s) have you considered?

Try to fix the above issues by exclusively using TypePath instead of TypeId and manually recreating the VTable struct. ctor is unavoidable, we just have to hope it won't be encountered in practice.

Additional context

Some discussion on Discord on bevy_dynamic_plugin, while I was working on #11622.

I think there is promise is dynamically loading plugins, so I would not be opposed to bevy_dynamic_plugin being re-introduced in the future, but I don't think the current implementation should be kept.

@alice-i-cecile alice-i-cecile added A-App Bevy apps and plugins X-Controversial There is active debate or serious implications around merging this PR P-Unsound A bug that results in undefined compiler behavior labels Feb 19, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 19, 2024

FYI @cart. I know you pushed back on this in #3893 in the past.

This is the result of a reasonable amount of design thinking. It is currently our best (and only) solution to the problem that doesn't require massive internal reworks and/or compromised UX. We are still free to design new things and I don't think keeping this in meaningfully limits that conversation. And I doubt we will see anything better that doesn't:

I think the landscape has meaningfully changed here on the editor side, between the existence of dextrous_developer and @coreh's remote protocol experiments. That said, I still don't have a good answer to the DLC use case you mentioned:

I would argue that this is helpful, for the exact scenario mentioned above (loading features compiled separately from each other at the start up of a pre-compiled app). That isn't particularly niche. In addition to making editor experiences better for minimal additional effort, this would also enable things like "loading a dlc downloaded from the internet / distributed separately from the main game". I consider that to be very attractive and very helpful.

Overall while I share these concerns I'd be fine to leave this around until we have a good sense of the architecture of the editor, and how editor extensions in particular will work.

@lee-orr
Copy link
Contributor

lee-orr commented Feb 19, 2024

I should note that I am biased here - as the maintainer of dexterous_developer - but I think the current version of this crate is worth revoking.

I also think that the solutions for dynamic loading in a development context should be different than the ones used for things like DLC and mods - the constraints around them are very different:

  • During development, things like rust version, nightly, and features are unlikely to change. And if they do - rebuild and you're good! So having some amount of instability there is more acceptable. In a released project, you really want a solid ABI for dynamic libraries, or to use IPC.
  • In addition, in development you might be OK with using some less optimized/performant build processes - which could open up some additional avenues. In a released project, those optimizations (whether around size or performance) become much more important.
  • Lastly, for development, the priority is being able to "hot-reload" a library for fast iteration. In a released project, you would often be turning these features on and off in a menu or even through a loader, and so they don't need to have the same state retention capacity - and might be able to handle full restarts.

I do think it might be worth looking at adding stabby to Bevy as an optional feature for ABI stability, and then we can use that as a basis for both the bevy plugin in dexterous developer (or for integrating that plugin into bevy) and for a separate approach for handling release-capable dynamic libraries for mods and DLC.

@BD103
Copy link
Member Author

BD103 commented Feb 21, 2024

I just opened #12029. While it does not deprecate dynamically_load_plugin, it does encourage the use of several alternatives which may be potentially safer.

github-merge-queue bot pushed a commit that referenced this issue May 20, 2024
# Objective

- The current implementation for dynamic plugins is unsound. Please see
#11969 for background and justification.
- Closes #11969 and closes #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
salvadorcarvalhinho pushed a commit to salvadorcarvalhinho/bevy that referenced this issue 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
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 P-Unsound A bug that results in undefined compiler behavior X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants