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

Support multiple heterogeneous backends at the same time #5243

Open
wants to merge 67 commits into
base: main
Choose a base branch
from

Conversation

xdoardo
Copy link
Contributor

@xdoardo xdoardo commented Nov 12, 2024

@xdoardo xdoardo linked an issue Nov 12, 2024 that may be closed by this pull request
@xdoardo xdoardo marked this pull request as ready for review November 20, 2024 17:02
@xdoardo xdoardo requested a review from theduke November 21, 2024 17:45
Adds a function to retrieve a unique identifier (backed by an
`AtomicU64`) for the top-level `Engine` type. The `id` is saved in the
`Engine` struct itself rather than types down the line. This fits better
with the current structure of the `sys` rt engines, which are defined in
the `wasmer_compiler` crate.
impl Default for RuntimeEngine {
#[allow(unreachable_code)]
fn default() -> Self {
#[cfg(feature = "sys")]
Copy link
Member

Choose a reason for hiding this comment

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

One thing is enabling a sys runtime, and other a default runtime being sys. I think we need a new feature that let you choose the default (eg: sys-default), that can't be activated when other default runtime (eg: js-default) or similar are activated (compiler error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a feature called sys-default. What would happen if sys is enabled but not sys-default?

Copy link
Member

Choose a reason for hiding this comment

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

Basically, we should not have default for RuntimeEngine if there's no default defined

Copy link
Member

Choose a reason for hiding this comment

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

If we don't want to have that because is a breaking change, then I'd have what you have now as a fallback, not as the main path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the js-default, jsc-default, v8-default, wamr-default and wasmi-default features. The Engine::default() function will be just as it is in this PR, but before the feature = "sys" cfgs I'll add a
#[cfg(feature = "sys-default")] for each runtime. Sounds good? Is this what you meant?

Copy link
Member

Choose a reason for hiding this comment

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

Yup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants