Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

API Discussion #3

Open
guybedford opened this issue Sep 6, 2019 · 7 comments
Open

API Discussion #3

guybedford opened this issue Sep 6, 2019 · 7 comments

Comments

@guybedford
Copy link
Contributor

guybedford commented Sep 6, 2019

Thought it would be worth getting the API discussion going here. The previous suggestion was a quick gist for discussion, thinking about it more I think it could be useful to:

  • Support raw buffer / Web Assembly.Module input
  • Not do any automatic internal caching of module compilation, and instead separating compilation from execution as Web Assembly itself does.
  • Simplify the start event model by separating init from exec.

Here's the adjusted suggestion for that approach:

import { createWASICommand } from 'wasi';

// async init (because Web Assembly)
// Support three overloads - path string | Buffer | WebAssembly.Module
const cmd = await createWASICommand('./path.wasm' : String);
const cmd = await createWASICommand(Buffer.from(bytes) : { buffer: ArrayBuffer });
const cmd = await createWASICommand(module: WebAssembly.Module);

const cmdProcess = cmd.exec(['args'], {
  env,  // defaults to an empty object
  modulePath, // defaults to the createWASICommand path string if provided
});

// same sort of events, properties and methods as child process
cmdProcess.stdin
cmdProcess.stdout
cmdProcess.stderr
cmdProcess.on('close', () => {});
cmdProcess.kill();

This is still very much a bikeshed so feel free to get those colours out further... :)

(Previous suggestion: https://gist.github.com/guybedford/890339e83e8cb1abc636a84b003f7a78 )

@devsnek
Copy link
Member

devsnek commented Sep 6, 2019

cc @sunfish in case you have any thoughts

@cjihrig
Copy link
Contributor

cjihrig commented Sep 6, 2019

@devsnek did you mean to cc @sunfishcode?

@devsnek
Copy link
Member

devsnek commented Sep 6, 2019

indeed I did :)

@devsnek
Copy link
Member

devsnek commented Sep 6, 2019

The goal with my PR was to avoid exposing an API entirely until things were more stable, and I still think that's a good approach, but it is rather awkward to set up. If we want to start with an API, I'd suggest branching off of VM and adding a new WASIModule class. From there all the various things a wasi module might want to do (evaluate directly, export functionality, etc) are well represented.

@sunfishcode
Copy link

I don't yet have a clear view on when to expose an API like this, but I can comment on some of the bikeshed colors ;-).

In addition to stdin and stdout, it'd be good to expose stderr.

const cmdProcess = cmd.exec(['args'], {
env, // defaults to process.env

I would ideally like to avoid having WASI programs becoming accustomed to inheriting the entire environment by default, because it often contains interesting information specific to the host it's on and the user. What would you think about having this default to empty, and requiting users to explicitly specify which environment variables they'd like to propagate through?

argv, // defaults to the createWASICommand path string if provided

What's the difference between this and the ['args'] passed above?

});

@guybedford
Copy link
Contributor Author

In addition to stdin and stdout, it'd be good to expose stderr.

Good point I've added that case.

What would you think about having this default to empty, and requiting users to explicitly specify which environment variables they'd like to propagate through?

If no environment was passed would we then just use an empty environment object? I suppose that would likely be ok in many scenarios actually?

What's the difference between this and the ['args'] passed above?

I suppose it is because the first argument is supposed to be the path to executable and when running these virtualized programs it's a common slip to forget to include the path or some dummy value in its place. So it was really just personal preference to avoid that slip by defaulting to the wasm path itself from compilation.

Child process in Node.js has modulePath and args as distinct from argv (https://nodejs.org/dist/latest-v12.x/docs/api/child_process.html#child_process_child_process_fork_modulepath_args_options). To try make this clearer I've explicitly used the name modulePath to match that, let me know if that seems clearer here?

@sunfishcode
Copy link

What would you think about having this default to empty, and requiting users to explicitly specify which environment variables they'd like to propagate through?

If no environment was passed would we then just use an empty environment object? I suppose that would likely be ok in many scenarios actually?

Yeah. WASI is all about applications by default being unable to do anything until you give them capabilities. That seems to translate to environment variables as, by default they don't get to know your HOSTNAME, USER, PWD, and everything else, unless you chose to give those to them.

To try make this clearer I've explicitly used the name modulePath to match that, let me know if that seems clearer here?

Yes, that's clarifies it!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants