[wasm] Use response files for `emcc` invocations by radical · Pull Request #51530 · dotnet/runtime

radical · 2021-04-19T23:33:37Z

This would avoid running into command max length issues on windows.

...
--js-library "C:\Users\Ankit Jain\.nuget\packages\microsoft.netcore.app.runtime.mono.browser-wasm\6.0.0-preview.5.21219.10\runtimes\browser-wasm\native\src\library_mono.js"
--js-library "C:\Users\Ankit Jain\.nuget\packages\microsoft.netcore.app.runtime.mono.browser-wasm\6.0.0-preview.5.21219.10\runtimes\browser-wasm\native\src\pal_random.js"
"C:\dev\w3\project\obj\Debug\net6.0\wasm\System.Private.CoreLib.dll.bc"
"C:\dev\w3\project\obj\Debug\net6.0\wasm\System.Text.Json.dll.bc"
"C:\dev\w3\project\obj\Debug\net6.0\wasm\System.Text.Encodings.Web.dll.bc"
...

radical · 2021-04-19T23:33:52Z

/cc @pranavkm

ghost · 2021-04-19T23:34:00Z

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

Issue Details

This would avoid running into command max length issues on windows.

Adds a new ExecWithResponseFile task

Fixes #51437 .

Author:	radical
Assignees:	-
Labels:	`arch-wasm`, `area-Build-mono`
Milestone:	-

lewing · 2021-04-20T00:30:43Z

I think you should consider the rsp files as build artifacts and split them up appropriately. I'm not sure a separate task is needed.

This adds response file for the linker command line, which will be the longest one. And it uses a known name for the response file. In future, once we have dependency checking, we can use such response files for that too. Fixes dotnet#51437 .

radical · 2021-04-20T00:48:27Z

I think you should consider the rsp files as build artifacts and split them up appropriately. I'm not sure a separate task is needed.

removed the task
and using a fixed response filename for the linker command line
for the rest, it can be added when we add dependency checking

radekdoulik

LGTM.

I was curious about the multiline response file. Tried to use it with emcc on windows and noticed the response file needs to be entered with full path. That is the case here, so I think it should work fine on windows. (<_EmccLinkerResponseFile>$(_WasmIntermediateOutputPath)emcc-link.rsp</_EmccLinkerResponseFile> , here the _WasmIntermediateOutputPath is normalized msbuild path)

src/mono/wasm/build/WasmApp.targets

Having the arguments one per line, also means that we don't have to quote the file paths. And it is more readable.

radical · 2021-04-21T01:45:29Z

Failing test on runtime (Libraries Test Run release mono Linux x64 Debug) , and runtime (Libraries Test Run release mono osx x64 Debug) is #51588

src/mono/wasm/build/WasmApp.targets

Suggested by Pranavkm: ``` .. their format does require whitespaces to be quoted: https://github.com/emscripten-core/emscripten/blob/main/tools/response_file.py#L16-L53. ```

lewing

Assuming this solves the immediate cmd length issue and the quoting works again this looks ok to me.

radical · 2021-04-21T20:13:24Z

Assuming this solves the immediate cmd length issue and the quoting works again this looks ok to me.

Added snippet of the what the rsp looks like to the description.

...
--js-library "C:\Users\Ankit Jain\.nuget\packages\microsoft.netcore.app.runtime.mono.browser-wasm\6.0.0-preview.5.21219.10\runtimes\browser-wasm\native\src\library_mono.js"
--js-library "C:\Users\Ankit Jain\.nuget\packages\microsoft.netcore.app.runtime.mono.browser-wasm\6.0.0-preview.5.21219.10\runtimes\browser-wasm\native\src\pal_random.js"
"C:\dev\w3\project\obj\Debug\net6.0\wasm\System.Private.CoreLib.dll.bc"
"C:\dev\w3\project\obj\Debug\net6.0\wasm\System.Text.Json.dll.bc"
"C:\dev\w3\project\obj\Debug\net6.0\wasm\System.Text.Encodings.Web.dll.bc"
...

lewing · 2021-04-21T23:47:01Z

/backport to release/6.0-preview4

github-actions · 2021-04-21T23:47:15Z

Started backporting to release/6.0-preview4: https://github.com/dotnet/runtime/actions/runs/772410348

radical requested review from akoeplinger and lewing April 19, 2021 23:33

radical requested a review from marek-safar as a code owner April 19, 2021 23:33

ghost added the area-Build-mono label Apr 19, 2021

radical added the arch-wasm WebAssembly architecture label Apr 19, 2021

radical force-pushed the emcc-rsp branch from 0d82605 to f63a9d1 Compare April 20, 2021 00:47

radekdoulik approved these changes Apr 20, 2021

View reviewed changes

pranavkm reviewed Apr 20, 2021

View reviewed changes

src/mono/wasm/build/WasmApp.targets Outdated Show resolved Hide resolved

pranavkm reviewed Apr 20, 2021

View reviewed changes

src/mono/wasm/build/WasmApp.targets Outdated Show resolved Hide resolved

[wasm] Emit emcc arguments, one per line in the rsp file

2f1076b

Having the arguments one per line, also means that we don't have to quote the file paths. And it is more readable.

radical requested review from pranavkm and radekdoulik April 21, 2021 03:25

runfoapp bot mentioned this pull request Apr 21, 2021

Test System.ComponentModel.Tests.ReflectionCachesUpdateHandlerTests.ReflectionCachesUpdateHandler_CachesCleared is failing on mono OSX x64 #51588

Closed

pranavkm reviewed Apr 21, 2021

View reviewed changes

src/mono/wasm/build/WasmApp.targets Outdated Show resolved Hide resolved

radical added 2 commits April 21, 2021 13:22

[wasm] Revert to quote file paths even in response file for emcc

ddc7918

Suggested by Pranavkm: ``` .. their format does require whitespaces to be quoted: https://github.com/emscripten-core/emscripten/blob/main/tools/response_file.py#L16-L53. ```

Merge remote-tracking branch 'origin/main' into emcc-rsp

fe3dfa8

lewing approved these changes Apr 21, 2021

View reviewed changes

lewing merged commit 5e957c4 into dotnet:main Apr 21, 2021

github-actions bot mentioned this pull request Apr 21, 2021

[release/6.0-preview4] [wasm] Use response files for emcc invocations #51658

Merged

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm] Use response files for `emcc` invocations#51530

[wasm] Use response files for `emcc` invocations#51530
lewing merged 4 commits intodotnet:mainfrom
radical:emcc-rsp

radical commented Apr 19, 2021 •

edited

Loading

Uh oh!

radical commented Apr 19, 2021

Uh oh!

ghost commented Apr 19, 2021

Uh oh!

lewing commented Apr 20, 2021 •

edited

Loading

Uh oh!

radical commented Apr 20, 2021

Uh oh!

radekdoulik left a comment

Uh oh!

Uh oh!

Uh oh!

radical commented Apr 21, 2021

Uh oh!

Uh oh!

lewing left a comment

Uh oh!

radical commented Apr 21, 2021

Uh oh!

lewing commented Apr 21, 2021

Uh oh!

github-actions bot commented Apr 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

radical commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radical commented Apr 19, 2021

Uh oh!

ghost commented Apr 19, 2021

Uh oh!

lewing commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radical commented Apr 20, 2021

Uh oh!

radekdoulik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

radical commented Apr 21, 2021

Uh oh!

Uh oh!

lewing left a comment

Choose a reason for hiding this comment

Uh oh!

radical commented Apr 21, 2021

Uh oh!

lewing commented Apr 21, 2021

Uh oh!

github-actions bot commented Apr 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

radical commented Apr 19, 2021 •

edited

Loading

lewing commented Apr 20, 2021 •

edited

Loading