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

[wasm] cleanup testing and host support in templates and tests #73785

Merged
merged 18 commits into from
Aug 15, 2022

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Aug 11, 2022

  • added new module exports dotnet, exit to dotnet.js
  • exit method is replacement for our internal set_exit_code
  • dotnet is a builder pattern with following usage
import { dotnet } from './dotnet.js'

dotnet
    .withDiagnosticTracing(false)
    .withApplicationArguments("dotnet", "is", "great!")
    .run()

The builder methods are

    withConfig(config: MonoConfig): DotnetHostBuilder
    withConfigSrc(configSrc: string): DotnetHostBuilder
    withApplicationArguments(...args: string[]): DotnetHostBuilder
    withEnvironmentVariable(name: string, value: string): DotnetHostBuilder
    withEnvironmentVariables(variables: { [i: string]: string; }): DotnetHostBuilder
    withVirtualWorkingDirectory(vfsPath: string): DotnetHostBuilder
    withDiagnosticTracing(enabled: boolean): DotnetHostBuilder
    withDebugging(level: number): DotnetHostBuilder
    create(): Promise<RuntimeAPI>
    run(): Promise<number>

There are also internal methods useful when testing.
withRuntimeOptions, withWaitingForDebugger, withElementOnExit, withConsoleForwarding, withExitCodeLogging and withModuleConfig

  • changed implementation of stdout flusing on NodeJS, that it would only delay the process.exit if the exit code is 0
  • updated templates
  • updated tests
  • fixed license headers

TODO

  • update WBT to use withElementOnExit, withConsoleForwarding, withExitCodeLogging as necessary for templates test
  • Update WasmAppHost to always pass parameters as query string and add withApplicationArgumentFromQuery
  • INTERNAL.require may not work (WS unit tests)
  • add readme to the templates
  • withAsyncNodeJsFlushOnExit as non-public opt-in nodejs flush on exit (enable optionally on windows only tests?)
  • public API withMainAssembly for setting main assembly name
  • create should return this.instance once it exists
  • fix CI build (possibly we may need to move everything to the post.ex)
  • dotnet.es6.extpost.js requires a change in the SDK

Contributes to #70892
Fixes #71047

@pavelsavara pavelsavara added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript labels Aug 11, 2022
@pavelsavara pavelsavara self-assigned this Aug 11, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

nothing yet

Author: pavelsavara
Assignees: pavelsavara
Labels:

NO-MERGE, NO-REVIEW, arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: -

# Conflicts:
#	src/mono/wasm/test-main.js
@pavelsavara pavelsavara changed the title [wasm] draft [wasm] cleanup testing and host support in templatest and tests Aug 12, 2022
@pavelsavara pavelsavara removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Aug 12, 2022
@maraf
Copy link
Member

maraf commented Aug 12, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

maraf and others added 5 commits August 12, 2022 22:19
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek lambdageek self-assigned this Aug 13, 2022
…pers

# Conflicts:
#	src/mono/wasm/runtime/dotnet.d.ts
#	src/mono/wasm/runtime/types.ts
@maraf
Copy link
Member

maraf commented Aug 14, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maraf
Copy link
Member

maraf commented Aug 14, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara marked this pull request as ready for review August 14, 2022 15:55
@pavelsavara pavelsavara requested a review from kg August 14, 2022 15:56
@maraf
Copy link
Member

maraf commented Aug 14, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

const { runtimeBuildInfo, setModuleImports, getAssemblyExports, runMain, getConfig } = await dotnet
.withConsoleForwarding()
.withElementOnExit()
.withModuleConfig({
Copy link
Member

Choose a reason for hiding this comment

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

why are we configuring some things with a literate '.withXxx' api here and then passing in a big config dictionary?

Copy link
Member

Choose a reason for hiding this comment

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

It's just for backward compatibility. We don't allow everything with those with* methods. Also this method is not "public" as it is not in the typescript public definition.

Copy link
Member Author

@pavelsavara pavelsavara Aug 14, 2022

Choose a reason for hiding this comment

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

We actually don't need any of those callbacks now. They could be removed from the sample. It's just showing that they are still there and working, in case somebody needs to go low level.

This could be further improved later and it should not block the PR, I think

Copy link
Member

Choose a reason for hiding this comment

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

WebHostBuilder* seems to go with Use* or Configure* rather than With*?

@lewing
Copy link
Member

lewing commented Aug 14, 2022

The failures here seem to be the linker related problems that are also in main.

withDiagnosticTracing(enabled: boolean): DotnetHostBuilder;
withDebugging(level: number): DotnetHostBuilder;
withMainAssembly(mainAssemblyName: string): DotnetHostBuilder;
withApplicationArgumentsFromQuery(): DotnetHostBuilder;
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it could have an optional string parameter that defaults to "args"

@lewing lewing merged commit 906ec1d into dotnet:main Aug 15, 2022
@maraf maraf changed the title [wasm] cleanup testing and host support in templatest and tests [wasm] cleanup testing and host support in templates and tests Aug 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2022
@pavelsavara pavelsavara deleted the wasm_host_helpers branch November 29, 2022 16:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm] Add readme about how to run the template
6 participants