-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Build WASM runtimes #35566
Build WASM runtimes #35566
Conversation
The actual build is not setup yet. |
@@ -137,6 +137,8 @@ | |||
Include="$(MonoArtifactsPath)\cross\*.*" /> | |||
<MonoIncludeFiles Condition="'$(TargetsMobile)' == 'true' or '$(TargetOS)' == 'Browser'" | |||
Include="$(MonoArtifactsPath)\include\**\*.*" /> | |||
<WasmDistFiles Condition="'$(TargetOS)' == 'Browser'" | |||
Include="$(MonoArtifactsPath)\wasm\runtimes\**\*.*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think wasm name is redundant in this context and it might be better to explicitly included as there is only a handful of files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The different kinds of files go to different places inside the nuget, so they need different property names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to \wasm\runtimes
not sure we need this if the package is wasm only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm yeah it's a bit redundant but since this is the source directory where the files get picked up rather than the destination in the nuget I don't care too much here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Shouldn't we be using Condition="'$(TargetsBrowser)' == 'true'"
here instead of Condition="'$(TargetOS)' == 'Browser'"
for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally yes, but that can be cleaned up later.
a92b9c9
to
c371e7c
Compare
0f58684
to
cca8a28
Compare
The wasm installer build needs EMSDK_PATH set as well. |
e681a00
to
d3f2741
Compare
This seems to work now. |
@akoeplinger please review |
@@ -137,6 +137,8 @@ | |||
Include="$(MonoArtifactsPath)\cross\*.*" /> | |||
<MonoIncludeFiles Condition="'$(TargetsMobile)' == 'true' or '$(TargetOS)' == 'Browser'" | |||
Include="$(MonoArtifactsPath)\include\**\*.*" /> | |||
<WasmDistFiles Condition="'$(TargetOS)' == 'Browser'" | |||
Include="$(MonoArtifactsPath)\wasm\runtimes\**\*.*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm yeah it's a bit redundant but since this is the source directory where the files get picked up rather than the destination in the nuget I don't care too much here.
Include="@(MonoIncludeFiles)"> | ||
<TargetPath>runtimes/$(PackageRID)/native/include/%(RecursiveDir)</TargetPath> | ||
</RuntimeFiles> | ||
|
||
<RuntimeFiles Condition="'$(TargetsBrowser)' == 'true'" | ||
Include="@(WasmDistFiles)"> | ||
<TargetPath>runtimes/$(PackageRID)/native/wasm/runtimes/%(RecursiveDir)</TargetPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the wasm/runtimes
in the path is redundant
…h to the make dependencies.
…ks might not exist yet.
No description provided.