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][wbt] Test that dotnet.js could be run from any current directory #69441

Merged
merged 10 commits into from Jul 28, 2022

Conversation

yamachu
Copy link
Contributor

@yamachu yamachu commented May 17, 2022

In #72275 we fixed locateFile to deal with relative file path or URL. We could also handle absolute path/URL. This PR is adding tests to prove it works.

@yamachu yamachu requested a review from marek-safar as a code owner May 17, 2022 16:14
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 17, 2022
yamachu added a commit to yamachu/azure-functions-nodejs-with-csharp that referenced this pull request May 17, 2022
Until then, replace `else t=r.name;` in dotnet.js with `else t=locateFile(r.name);`
@yamachu yamachu changed the title Use localeFile to resolve path for assets outside of remoteSources Use locateFile to resolve path for assets outside of remoteSources May 17, 2022
@marek-safar marek-safar added the arch-wasm WebAssembly architecture label May 17, 2022
@ghost
Copy link

ghost commented May 17, 2022

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

Issue Details

Use locateFile to resolve path for assets outside of remoteSources, so that we can run from a different path than locate that dotnet.js exists

ref: #69259

I followed the repro step described in the Issue ( #69259 ) and confirmed that it works.

Referring to the mono-config.json generation task, I found that the file output destination was fixed at AppDir based path.
I therefore decided that I could use a locateFile based from scriptDirectory ( https://github.com/emscripten-core/emscripten/blob/e1d97f55d12ed63dc30e547c818af9a63be0b976/src/shell.js#L155 ).

https://github.com/dotnet/runtime/blob/3f0c2cc88dc8efdde04f422bb05ef13719ce6aa8/src/tasks/WasmAppBuilder/WasmAppBuilder.cs

Author: yamachu
Assignees: -
Labels:

arch-wasm, community-contribution

Milestone: -

@pavelsavara
Copy link
Member

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing lewing requested a review from thaystg May 17, 2022 16:52
@pavelsavara
Copy link
Member

hi @yamachu, thanks for the contribution!
At first look I like it, it brings more consistency to locating assets.

It would be great to add unit test to cover running V8 and NodeJS from different folder.
I think we are setting the folder here

I also wonder what would happen to web app, which has dotnet.js on path relative to window.location
I think we don't have unit test for that either.
(Note that this loader is not used by Blazor, they have their own)

@yamachu
Copy link
Contributor Author

yamachu commented May 18, 2022

Thanks for the review!

I have set up an environment to run the tests you presented, so I rewrote the working directory and ran the tests.
But, the working directory in run-v8.sh has changed and v8 is having problems finding test-main.js.
There is also a module resolution issue with js, so it appears that a drastic rewrite of the test environment is needed...

@yamachu
Copy link
Contributor Author

yamachu commented May 18, 2022

I have not yet been to verify this for the web app.
Looking at the emscripten implementation, it starts from the URL (window.location.href) where the execution took place.
This is very different from the behavior of path resolution in Node.

In a web environment, it may be desirable to override the locateFile, as in the following sample code.
For example, if you consider the case where you want to distribute dotnet.js or managed assets on a CDN or similar, the locateFile behavior provided by emscripten will not work.
https://github.com/dotnet/runtime/blob/main/src/mono/sample/wasm/browser-nextjs/components/deepThought.js

However, I think it is also possible to support this by utilizing remoteSources in addition to this locateFile change.
https://github.com/dotnet/runtime/blob/10883a7bae7fbd99b1edaa77d4ea4a5a3766114d/src/tasks/WasmAppBuilder/WasmAppBuilder.cs

Currently, only icu files support remoteSources, but I think that by making all assets support it, it will be possible to execute in a way that does not depend on the behavior of the locateFile.

@yamachu yamachu changed the title Use locateFile to resolve path for assets outside of remoteSources Use locateFile to resolve path for assets outside of AppBundle May 21, 2022
@yamachu yamachu changed the title Use locateFile to resolve path for assets outside of AppBundle Use locateFile to resolve path for assets outside of remoteSources May 21, 2022
@pavelsavara pavelsavara requested a review from radical May 23, 2022 08:27
@yamachu

This comment was marked as outdated.

@ghost
Copy link

ghost commented May 29, 2022

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Use locateFile to resolve path for assets outside of remoteSources, so that we can run from a different path than locate that dotnet.js exists

ref: #69259

I followed the repro step described in the Issue ( #69259 ) and confirmed that it works.

Referring to the mono-config.json generation task, I found that the file output destination was fixed at AppDir based path.
I therefore decided that I could use a locateFile based from scriptDirectory ( https://github.com/emscripten-core/emscripten/blob/e1d97f55d12ed63dc30e547c818af9a63be0b976/src/shell.js#L155 ).

https://github.com/dotnet/runtime/blob/3f0c2cc88dc8efdde04f422bb05ef13719ce6aa8/src/tasks/WasmAppBuilder/WasmAppBuilder.cs

Author: yamachu
Assignees: pavelsavara
Labels:

arch-wasm, area-Infrastructure-mono, community-contribution

Milestone: -

@yamachu yamachu requested a review from lewing as a code owner July 4, 2022 16:51
@pavelsavara
Copy link
Member

@yamachu
I took some of your ideas into #72275.
I would appreciate if you could test that it solves your scenario.

After it's merged, we could rebase this PR and use the unit test you created.

@pavelsavara
Copy link
Member

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Jul 26, 2022

string binDir = GetBinDir(baseDir: _projectDir!, config: buildArgs.Config);
string appBundleDir = Path.Combine(binDir, "AppBundle");
string tmpBundleDirName = "AppBundleTmp";
string tmpBundleDir = Path.Combine(binDir, tmpBundleDirName);
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of creating this tmpBundleDir in a completely different location, like under Path.GetTempPath()? This would ensure that it isn't dependent on anything in the binDir.
And make sure to add id, and the project name to the dir name, so the different runs don't overlap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a good method in Node and V8 environments!
I' ll change that style.

However, browsers like Chrome cannot explore the parent directory, so I would like to keep the current style.

@radical
Copy link
Member

radical commented Jul 27, 2022

Don't merge yet. The test needs some changes. I will get those working locally, and share that tomorrow.

@radical
Copy link
Member

radical commented Jul 27, 2022

So, one of the problems with the WasmRunOutOfAppBundle test is that we won't be able to run dotnet xharness outside in a temporary directory. That can be worked around by creating a tool manifest there, but then we have to get the xharness version from the repo to helix. This applies to all the hosts.

IIUC, the intention here is:

  1. v8, node - start from a directory which is not the AppBundle directory. In which case we can use the parent directory.
  2. chrome - the web server can be started in the parent directory too.

This is what I came up with: https://gist.github.com/radical/18d60949c789cf98f8b175c30096b683 . Feel free to add this to your PR.

The test output has:

  • Running v8 --expose_wasm ./AppBundle/test-main.js ...
  • Running node ./AppBundle/test-main.js ...

Make sure to confirm that it is indeed testing what is intended here. AFAIU, it does that.

@radical
Copy link
Member

radical commented Jul 27, 2022

@yamachu Thank you for your contribution! Great job 👍

@yamachu
Copy link
Contributor Author

yamachu commented Jul 27, 2022

@pavelsavara @radical
Thanks for all your support!

@radical
Copy link
Member

radical commented Jul 28, 2022

Failures are unrelated.

@radical
Copy link
Member

radical commented Jul 28, 2022

@yamachu This is only adding the --config-src param to test-main.js, which is not used by any user apps. Was that the intention here? I don't see the reason for adding that, since tests run in a pretty controlled environment.

@yamachu
Copy link
Contributor Author

yamachu commented Jul 28, 2022

This addition is to verify that config-src could be resolved when passed as an absolute path.
ref: #69441 (comment)

@radical radical merged commit 79d1988 into dotnet:main Jul 28, 2022
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Infrastructure-mono community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants