Added two methods for loading embedded .wasm and .wat

[redradist]

No description provided.

[peterhuene]

Hi @redradist. Thank you for the pull request!

I'm not entirely convinced that these methods are needed in the Wasmtime API. Could you explain a little more about your use case?

I feel like the LoadModule(string name, byte[] bytes) overload already satisfies this use case of loading a module from arbitrary bytes (regardless of where the module was stored).

Additionally, calling Assembly.GetCallingAssembly seems like the wrong thing to do rather than, perhaps, accepting a Stream the caller provides.

If we do decide it makes sense to include these methods, we will need some unit tests implemented that covers them.

[redradist]

@peterhuene

Hi @redradist. Thank you for the pull request!

I'm not entirely convinced that these methods are needed in the Wasmtime API. Could you explain a little more about your use case?

I feel like the LoadModule(string name, byte[] bytes) overload already satisfies this use case of loading a module from arbitrary bytes (regardless of where the module was stored).

Additionally, calling Assembly.GetCallingAssembly seems like the wrong thing to do rather than, perhaps, accepting a Stream the caller provides.

If we do decide it makes sense to include these methods, we will need some unit tests implemented that covers them.

The reason I need them because I want to store in my Xamarin application *.wasm *.wat files and then during startup or by some event to load the module
But LoadModule could not work with EmbeddedResource-s ... (
That is why I have create pull-request

[peterhuene]

But LoadModule could not work with EmbeddedResource-s

True, it will not work with them directly, but I don't see why it need to when you can load the data yourself and then call the existing overloads of LoadModule and LoadModuleText, much like what these new methods are doing.

However, I can get behind overloads for LoadModule and LoadModuleText that take a Stream rather than doing the loading of the resource from the calling assembly. For LoadModuleText, it would assume UTF-8 encoding of the text. Would that work for you? It would cut down on how much code you need to to implement on your side of things.

If that works, would you also mind adding a test or two of calling the new overloads? I can help write the tests if you're unfamiliar with xunit.

[redradist]

@peterhuene

But LoadModule could not work with EmbeddedResource-s

True, it will not work with them directly, but I don't see why it need to when you can load the data yourself and then call the existing overloads of LoadModule and LoadModuleText, much like what these new methods are doing.

However, I can get behind overloads for LoadModule and LoadModuleText that take a Stream rather than doing the loading of the resource from the calling assembly. For LoadModuleText, it would assume UTF-8 encoding of the text. Would that work for you? It would cut down on how much code you need to to implement on your side of things.

If that works, would you also mind adding a test or two of calling the new overloads? I can help write the tests if you're unfamiliar with xunit.

Yes, I agree, it would be possible to load bytes externally and provide this data of bytes to either LoadModule or LoadModuleText, but it requires for user to make not trivial actions ...
From my point of view this functions will simplify developers to work with wasmtime ...

If you agree I will add unittests and update pull-request ...
Also what is your suggestion where to put tests for this methods ?

[redradist]

@peterhuene
By some reason tests now is not passed ...
But I have not touched any tests ... yet ;)

Could you support ?

[peterhuene]

Hi @redradist. Sorry for the delayed response.

It appears there's an upstream bug in Wasmtime that is causing the CI failure. I have identified the bug and have a fix. When that fix is merged, CI should automatically be fixed once rerun.

[redradist]

peterhuene

Hi @redradist. Sorry for the delayed response.

It appears there's an upstream bug in Wasmtime that is causing the CI failure. I have identified the bug and have a fix. When that fix is merged, CI should automatically be fixed once rerun.

Okay, thanks for response !!

Any suggestion where to put tests for this functionality ?
As you can see I add EmbeddedModule and in this case I will reuse a lots of available tests added for Module ...
But what to do with testing *.wasm files ?

[redradist]

@peterhuene
Is there any update regarding the issue with tests ?
I would like to complete this feature and merge it ...

[peterhuene]

We're still waiting on upstream Wasmtime PR to address the failure (bytecodealliance/wasmtime#1600).

Regarding this PR, I would still prefer not to have the resource loading code in the Wasmtime assembly; instead, we should add Stream overloads for LoadModule and LoadModuleText which would enable a more generic solution and prevent having to call GetCallingAssembly.

[peterhuene]

I don't think we want any embedded resource loading code in the Wasmtime assembly itself; to allow for generic loading from byte streams, can we instead change this PR to add Stream based overloads to LoadModule and LoadModuleText?

[redradist]

@peterhuene

I don't think we want any embedded resource loading code in the Wasmtime assembly itself; to allow for generic loading from byte streams, can we instead change this PR to add Stream based overloads to LoadModule and LoadModuleText?

You mean to add the following methods ?:

public Module LoadModule(Stream stream)
{
  // ...
}

public Module LoadEmbeddedModule(Stream stream)
{
  // ...
}

Actually I like this even more 'cause it more flexible solution ))

Okay, I will change implementation to Stream if you mean that

[peterhuene]

Hi @redradist. That's correct, except it would be LoadModuleText(Stream stream) as well. There wouldn't be a LoadEmbeddedModule method as the caller would be responsible for getting the stream from their embedded resource. While it would mean there's a little more work for the caller to do, it would keep the .NET API as flexible as possible without having to rely on calling GetCallingAssembly.

[redradist]

@peterhuene

Hi @redradist. That's correct, except it would be LoadModuleText(Stream stream) as well. There wouldn't be a LoadEmbeddedModule method as the caller would be responsible for getting the stream from their embedded resource. While it would mean there's a little more work for the caller to do, it would keep the .NET API as flexible as possible without having to rely on calling GetCallingAssembly.

I did it, but the method become such simple:

        public Module LoadModuleText(string moduleName, Stream moduleStream)
        {
            using (StreamReader reader = new StreamReader(moduleStream))
            {
                return LoadModuleText(moduleName, reader.ReadToEnd());
            }
        }
        public Module LoadModule(string moduleName, Stream moduleStream)
        {
            using (StreamReader reader = new StreamReader(moduleStream))
            {
                using (MemoryStream ms = new MemoryStream())
                {
                    reader.BaseStream.CopyTo(ms);
                    return LoadModule(moduleName, ms.ToArray());
                }
            }
        }

that I do not know actually if it is needed at all

[peterhuene]

While the methods are simple, it's good to have Stream overloads when also accepting byte[]. It enables users to simply pass in a network stream, file stream, or a stream from an embedded resource without having to first read it to memory. Additionally, it might be possible for Wasmtime, in the future, to incrementally load a module from a stream rather than buffered memory; having this overload would allow for the possibility of internal implementation changes that would reduce memory consumption.

[redradist]

@peterhuene
I have changed API as you requested ;)

[peterhuene]

@redradist I've rebased your branch to be the latest from the new default main branch.

You'll want to rebase your local branch from your origin's branch: git pull --rebase origin extend-host-api.

The commit I added was just a little cleanup up the two new methods you added, plus a test case that loads a wasm and wat file as an embedded resource, per your use case of these methods.

Once the CI goes green, I'll merge this.

[peterhuene]

@redradist Thanks very much for both your work on this as well as your patience getting this merged!

[peterhuene]

Oh, I plan on releasing a new version of the NuGet package with these changes soon, once I update the API to address #9.