feat: update to WIT-enabled wasmCloud #605

rvolosatovs · 2023-04-07T14:15:15Z

Feature or Problem

Update to NIF to wasmCloud/wasmCloud#300

Related Issues

wasmCloud/wasmCloud#300

Release Information

Consumer Impact

Testing

Built on platform(s)

Tested on platform(s)

Unit Test(s)

Acceptance or Integration

Manual Verification

autodidaddict

LGTM but I think there are some opportunities to rename some things to be more clear and disambiguated.

Also I can't tell from mobile but make sure all the integration tests pass on these new nif changes

autodidaddict · 2023-04-07T14:22:50Z

host_core/native/hostcore_wasmcloud_native/Cargo.toml

@@ -10,8 +10,7 @@ path = "src/lib.rs"
 crate-type = ["dylib"]

 [dependencies]
-# Patched temporarily to fix TinyGo modules, branch will be removed in v0.63.0
-wasmcloud = { git = "https://github.com/wasmcloud/wasmcloud", branch = "fix/0.62-tinygo"}
+wasmcloud-host = { git = "https://github.com/rvolosatovs/wasmcloud", branch = "feat/wit"}


Calling this wasmCloud host is confusing as there is a full elixir project in this repo called wasmCloud host (the washboard).

Also, we shouldn't be depending on your fork.

This is a draft and the fork is used precisely because wasmCloud/wasmCloud#300 is not merged yet. There's nothing else to depend upon to get this up

autodidaddict · 2023-04-07T14:23:56Z

host_core/native/hostcore_wasmcloud_native/src/wasmruntime.rs

+}
+
+#[derive(Clone)]
+struct ElixirHandlerArc(Arc<ElixirHandler>);


I know naming is hard but I'd rather not put a data type like arc as a suffix on a struct.

Could you suggest a better option?

It was called ElixirHandler before. The fact that the inner element is an arc shouldn't be advertised in the name.

You could call it a handler reference or elixir handler reference.. just something that doesn't have a data type in the name.

But what would the inner struct be called then? ElixirHandlerInner? I really don't see how that's better.

The only option to avoid this is to store an unnamed tuple in an Arc, but that'd be very difficult to maintain and use

I'm also not sure what caused the need for the arc wrapper around the elixir handler. Since I'm on a phone deferring to Taylor for thoughts on the abstraction and name

not sure what caused the need for the arc wrapper around the elixir handler

Mainly, wasmtime/wit-bindgen design drawbacks, namely the fact that the host trait implementations take &mut self parameter, therefore forcing this pattern on us

But shouldn't we be insulating consumers of our rust wasmCloud library from design changes and decisions of the underlying libraries? We shouldn't have to refactor the otp nif every time wasmtime makes an internal change... Isolating the host from this kind of churn is actually the primary purpose of the rust wasmCloud library.

not sure what caused the need for the arc wrapper around the elixir handler

Mainly, wasmtime/wit-bindgen design drawbacks, namely the fact that the host trait implementations take &mut self parameter, therefore forcing this pattern on us

If they force this on our wasmCloud library then the wasmCloud library should not pass this along to its consumers. It is designed specifically to prevent this

Sure, eventually we should think about this, depending on the state of things in the future - right now it's way too early to build these abstractions.

I disagree. Our job with libwasmcloud is to isolate consumers from wasmtime and its volatility. It's our job to come up with a more stable abstraction than the raw wasmtime interface.

autodidaddict · 2023-04-07T14:54:18Z

Bottom line: if we have to refactor the nif every time wasmtime changes an API, then libwasmcloud is not doing its job.

rvolosatovs · 2023-04-07T15:06:21Z

Bottom line: if we have to refactor the nif every time wasmtime changes an API, then libwasmcloud is not doing its job.

Again, that comes in the future. We will reach this point some day, but now is not the right time to build these abstractions (just like the handler abstraction API was prematurely built earlier and replaced here). It's much more dangerous and costly to build the wrong abstraction, rather than pay a minuscule cost of changing a few lines of code very infrequently (if at all). Please note that "libwasmcloud" is not even 0.1.0 yet and there will be breaking changes going forward. We will follow semantic versioning completely, but now it's way too early in the lifecycle of that project to do that.

Making sure these interfaces (WIT traits, WASI etc.) are actually usable and backwards compatible is Wasmtime/Bytecode Alliance's job, not ours.

What changes are you afraid of?

Eventually, the "Arc" trick will not be necessary anymore - the (wit-bindgen generated) traits should take &self rather than &mut self as parameter - changing this should not take more than 10 seconds with a simple search-and-replace. Removing the Arc is a possibility in that case, but not a requirement - the existing implementation will keep working just the same way it did.

Next, there's the question of WIT interfaces being updated - e.g. a new method is added. Again, this maintenance burden belongs upstream - it's upstream's job to ensure backwards compatibility, e.g. ensure there are default implementations for any new methods added.

If upstream fails it's job and introduces breaking changes, then that's the point where we can abstract these traits ourselves, i.e. instead of reexporting wit-bindgen-generated traits, export our own with the same name and API. At this point, nothing is broken, so I do not see a reason (on the contrary, I see reasons not to) fix it

brooksmtownsend · 2023-05-16T15:24:27Z

@rvolosatovs looks like we've gone through a few changes but settled that this PR is something we should do, would you mind providing a status update here on if this should stay draft, be replaced by another PR, etc?

rvolosatovs · 2023-05-30T16:45:33Z

@rvolosatovs looks like we've gone through a few changes but settled that this PR is something we should do, would you mind providing a status update here on if this should stay draft, be replaced by another PR, etc?

I think we should keep this until https://github.com/wasmCloud/wasmcloud has reached 0.1 or there are features we explicitly need from the library and then get back to this and update to recent changes. We don't get anything by doing this now, but there's a lot of work that will be useful once we do

cc @autodidaddict

autodidaddict · 2023-06-02T12:18:47Z

We don't get anything by doing this now, but there's a lot of work that will be useful once we do

cc @autodidaddict

Don't we get the ability for the OTP host to execute modules and components built using the latest standards?

autodidaddict · 2023-06-14T18:44:35Z

The version of provider-archive should be bumped to 0.7.0

Signed-off-by: Roman Volosatovs <[email protected]>

autodidaddict · 2023-06-15T15:10:36Z

Thanks for the provider-archive bump 👍

autodidaddict · 2023-06-15T17:43:30Z

@rvolosatovs when can this be taken out of draft and made ready to merge so we can use the new dependencies?

rvolosatovs · 2023-06-15T17:46:03Z

@rvolosatovs when can this be taken out of draft and made ready to merge so we can use the new dependencies?

the NIF segfaults on startup, someone has to debug and fix that, I can take a look tomorrow, but if you can figure out the issue, then please go ahead

stale · 2023-08-14T17:57:07Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this has been closed too eagerly, please feel free to tag a maintainer so we can keep working on the issue. Thank you for contributing to wasmCloud!

rvolosatovs force-pushed the feat/wit branch from e0fca2e to 3e53415 Compare April 7, 2023 14:17

rvolosatovs mentioned this pull request Apr 7, 2023

feat: implement builtin capability providers via WIT wasmCloud/wasmCloud#300

Merged

10 tasks

autodidaddict requested changes Apr 7, 2023

View reviewed changes

rvolosatovs force-pushed the feat/wit branch from fc73fa1 to 747490f Compare April 7, 2023 14:33

rvolosatovs force-pushed the feat/wit branch 3 times, most recently from d3200fa to 2a7e47c Compare June 14, 2023 16:59

rvolosatovs mentioned this pull request Jun 14, 2023

build: remove wit-deps build scripts wasmCloud/wasmCloud#359

Merged

10 tasks

feat: update to WIT-enabled wasmCloud

5de23e9

Signed-off-by: Roman Volosatovs <[email protected]>

rvolosatovs force-pushed the feat/wit branch from 2a7e47c to a881aaf Compare June 15, 2023 15:07

rvolosatovs force-pushed the feat/wit branch from a881aaf to 5de23e9 Compare June 15, 2023 16:58

stale bot added the stale label Aug 14, 2023

stale bot closed this Aug 21, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update to WIT-enabled wasmCloud #605

feat: update to WIT-enabled wasmCloud #605

rvolosatovs commented Apr 7, 2023

autodidaddict left a comment

autodidaddict Apr 7, 2023

rvolosatovs Apr 7, 2023

autodidaddict Apr 7, 2023

rvolosatovs Apr 7, 2023

autodidaddict Apr 7, 2023 •

edited

Loading

rvolosatovs Apr 7, 2023

autodidaddict Apr 7, 2023

rvolosatovs Apr 7, 2023

autodidaddict Apr 7, 2023

autodidaddict Apr 7, 2023

rvolosatovs Apr 7, 2023 •

edited

Loading

autodidaddict Apr 7, 2023

autodidaddict commented Apr 7, 2023

rvolosatovs commented Apr 7, 2023 •

edited

Loading

brooksmtownsend commented May 16, 2023

rvolosatovs commented May 30, 2023

autodidaddict commented Jun 2, 2023

autodidaddict commented Jun 14, 2023

autodidaddict commented Jun 15, 2023

autodidaddict commented Jun 15, 2023

rvolosatovs commented Jun 15, 2023

stale bot commented Aug 14, 2023

feat: update to WIT-enabled wasmCloud #605

feat: update to WIT-enabled wasmCloud #605

Conversation

rvolosatovs commented Apr 7, 2023

Feature or Problem

Related Issues

Release Information

Consumer Impact

Testing

Unit Test(s)

Acceptance or Integration

Manual Verification

autodidaddict left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autodidaddict Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvolosatovs Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autodidaddict commented Apr 7, 2023

rvolosatovs commented Apr 7, 2023 • edited Loading

brooksmtownsend commented May 16, 2023

rvolosatovs commented May 30, 2023

autodidaddict commented Jun 2, 2023

autodidaddict commented Jun 14, 2023

autodidaddict commented Jun 15, 2023

autodidaddict commented Jun 15, 2023

rvolosatovs commented Jun 15, 2023

stale bot commented Aug 14, 2023

autodidaddict Apr 7, 2023 •

edited

Loading

rvolosatovs Apr 7, 2023 •

edited

Loading

rvolosatovs commented Apr 7, 2023 •

edited

Loading