Skip to content

[wasm] Use rsp file for emcc default flags, and a props file instead of txt - #52941

Merged
radical merged 9 commits into
dotnet:mainfrom
radical:wasm-emcc-rsp
May 28, 2021
Merged

radical merged 9 commits into
dotnet:mainfrom
radical:wasm-emcc-rsp

Conversation

@radical

@radical radical commented May 18, 2021 •

Copy link
Copy Markdown
Member

emcc-default.rsp:

  • this replaces emcc-flags.txt which was generated at wasm build time
    as the default set of flags.

Emcc.props:

  • this replaces emcc-version.txt which was generated at wasm build
    time, and contained the output of emcc --version, eg:

    emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.12 (d0e647bf266caad50943e78c9841e05e9c499a5d)

  • Instead of this, we now generate Emcc.props, which has:

    $(RuntimeEmccVersionRaw) - full version string (emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.12 (d0e647bf266caad50943e78c9841e05e9c499a5d))
    $(RuntimeEmccVersion) - parsed version (2.0.12)
    $(RuntimeEmccVersionHash) - parsed hash (d0e647bf266caad50943e78c9841e05e9c499a5d)

  • these might be useful for nugets with native libraries for use with
    wasm, for example

  • Also, extracted WasmApp.Native.targets from WasmApp.targets

@ghost

ghost commented May 18, 2021

Copy link
Copy Markdown

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.

@radical radical added the arch-wasm WebAssembly architecture label May 18, 2021
@ghost

ghost commented May 18, 2021

Copy link
Copy Markdown

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

Issue Details

WIP!

Author: radical
Assignees: -
Labels:

arch-wasm

Milestone: -

radical added 3 commits May 19, 2021 17:21
`emcc-default.rsp`:
- this replaces `emcc-flags.txt` which was generated at wasm build time
  as the default set of flags.

`Emcc.props`:
- this replaces `emcc-version.txt` which was generated at wasm build
  time, and contained the output of `emcc --version`, eg:

  `emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.12 (d0e647bf266caad50943e78c9841e05e9c499a5d)`

- Instead of this, we now generate `Emcc.props`, which has:

    `$(RuntimeEmccVersionRaw)`  - full version string (`emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.12 (d0e647bf266caad50943e78c9841e05e9c499a5d)`)
    `$(RuntimeEmccVersion)`     - parsed version (`2.0.12`)
    `$(RuntimeEmccVersionHash)` - parsed hash (`d0e647bf266caad50943e78c9841e05e9c499a5d`)

- these might be useful for nugets with native libraries for use with
  wasm, for example

- The default options being used in the `Makefile` have been moved to
  `wasm.proj`, and used to populate the new files

- this tries to retain the same flags as are being used currently, on
  various platforms (eg. for windows the optimization flags differ)
- this is being done mainly to aid readability, and clarity
- a new target `WasmBuildNativeOnly` has been added, which should
  (eventually) be usable before `publish`.
  - this commit makes the changes for it, but it hasn't been tested in
    it's final form
@radical
radical marked this pull request as ready for review May 19, 2021 21:35
@radical
radical requested a review from marek-safar as a code owner May 19, 2021 21:35
@radical

radical commented May 19, 2021

Copy link
Copy Markdown
Member Author

Maybe RuntimeEmccVersion should be EmccVersion?

Comment thread src/mono/sample/wasm/wasm.mk Outdated
Comment thread src/mono/wasm/Makefile Outdated
@radical
radical requested a review from radekdoulik May 21, 2021 02:47
Comment thread src/mono/wasm/wasm.proj Outdated

@radekdoulik radekdoulik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lewing
lewing previously requested changes May 21, 2021

@lewing lewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't WasmApp.Native.targets need to be added to the pkgproj?

@radical

radical commented May 22, 2021

Copy link
Copy Markdown
Member Author

Doesn't WasmApp.Native.targets need to be added to the pkgproj?

Good catch! thanks, updated.

@radical
radical requested a review from lewing May 22, 2021 05:15
@lewing

lewing commented May 23, 2021

Copy link
Copy Markdown
Member

I'm a little worried this was green before adding the files. We keep having problems like this.

@radical

radical commented May 23, 2021

Copy link
Copy Markdown
Member Author

I'm a little worried this was green before adding the files. We keep having problems like this.

I agree. I was thinking of using the nugets with the wasm.build tests, to start with. It won't be a real workload based test though, since the sdk used is older, so probably wouldn't be prudent to depend on that for workload.

@lewing

lewing commented May 23, 2021

Copy link
Copy Markdown
Member

We should strive to find problems as close to the source as practical, that doesn't mean catching everything but it does mean checking for patterns we've seen before.

Comment thread src/mono/wasm/wasm.proj Outdated
Co-authored-by: Larry Ewing <lewing@microsoft.com>
<PlatformManifestFileEntry Include="emcc-flags.txt" IsNative="true" />
<PlatformManifestFileEntry Include="emcc-version.txt" IsNative="true" />
<PlatformManifestFileEntry Include="emcc-default.rsp" IsNative="true" />
<PlatformManifestFileEntry Include="Emcc.props" IsNative="true" />

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can runtime packs include a props file that will be automatically loaded?

</Target>


<Target Name="_SetupEmscripten">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we move the base emscripten detection to the WorkloadManifest.targets where it will always be available (and knows how to require the workload when appropriate)?

<!-- Adding optimization flag at the top, so it gets precedence -->
<_EmccLDFlags Include="$(EmccLinkOptimizationFlag)" />
<_EmccLDFlags Include="@(_EmccCommonFlags)" />
<_EmccLDFlags Include="-s TOTAL_MEMORY=536870912" />

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should the value here be a property?

@lewing
lewing dismissed their stale review May 27, 2021 21:38

Blocking issues have been resolved

@radical radical mentioned this pull request May 28, 2021
1 of 3 tasks
@radical

radical commented May 28, 2021

Copy link
Copy Markdown
Member Author

Suggestions being tracked in #53394 .

@radical
radical merged commit 6722c0b into dotnet:main May 28, 2021
@radical
radical deleted the wasm-emcc-rsp branch May 28, 2021 00:21
@ghost ghost locked as resolved and limited conversation to collaborators Jun 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasm WebAssembly architecture area-Build-mono

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants