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.

Sign up for GitHub

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

Jump to bottom

[wasm] Use response files for emcc invocations #51530

Merged
merged 4 commits into from
Apr 21, 2021
Merged

[wasm] Use response files for emcc invocations #51530

merged 4 commits into from
Apr 21, 2021

Conversation

radical
Copy link
Member

@radical radical commented Apr 19, 2021 •
edited
Loading

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

Fixes #51437 .

...
--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
Copy link
Member Author

radical commented Apr 19, 2021

/cc @pranavkm

@radical radical added the arch-wasm WebAssembly architecture label Apr 19, 2021
@ghost
Copy link

ghost commented Apr 19, 2021

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
Copy link
Member

lewing commented Apr 20, 2021 •
edited
Loading

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.

@radical
[wasm] Use response files for emcc invocations
f63a9d1 
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
Copy link
Member Author

radical commented Apr 20, 2021

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
radekdoulik approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@radekdoulik radekdoulik left a comment

Choose a reason for hiding this comment

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

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)

@radical
[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
Copy link
Member Author

radical commented Apr 21, 2021

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

@runfoapp runfoapp bot mentioned this pull request Apr 21, 2021
Closed
lewing
lewing approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

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

@radical
Copy link
Member Author

radical commented Apr 21, 2021

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 lewing merged commit 5e957c4 into dotnet:main Apr 21, 2021
@lewing
Copy link
Member

lewing commented Apr 21, 2021

/backport to release/6.0-preview4

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot mentioned this pull request Apr 21, 2021
Merged
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pranavkm pranavkm pranavkm left review comments

@lewing lewing lewing approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger akoeplinger is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar

@radekdoulik radekdoulik Awaiting requested review from radekdoulik

Assignees
No one assigned
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

WasmApp.targets on Windows can run in to cmd max length issues
5 participants
@radical @lewing @pranavkm @radekdoulik @karelz