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] Extract src/mono/browser from src/mono/wasm #95940

Merged
merged 7 commits into from
Dec 19, 2023
Merged

[wasm] Extract src/mono/browser from src/mono/wasm #95940

merged 7 commits into from
Dec 19, 2023

Conversation

radical
Copy link
Member

@radical radical commented Dec 13, 2023 •
edited
Loading

  • Wasm.Build.Tests is still in src/mono/wasm, mainly because it has lot of common code for build tests. And this will share more code with wasi-build-tests in future PRs.
  • This PR adds a src/mono/browser, and then moves some browser specific directories from src/mono/wasm to it.
  • Then the corresponding references to the paths are updated in all the places in the repo.

Includes changes based on review feedback from @ maraf, and @ pavelsavara

@radical radical added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it arch-wasm WebAssembly architecture area-Build-mono labels Dec 13, 2023
@ghost
Copy link

ghost commented Dec 13, 2023

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

Issue Details

TODO:

  • CI triggers
Author: radical
Assignees: -
Labels:

NO-MERGE, NO-REVIEW, arch-wasm, area-Build-mono
Milestone: -

@ghost ghost assigned radical Dec 13, 2023
This was referenced Dec 14, 2023
Closed
Open
Open
@radical
Copy link
Member Author

radical commented Dec 14, 2023

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Dec 14, 2023
Closed
@radical
Copy link
Member Author

radical commented Dec 14, 2023

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Dec 18, 2023

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

maraf
maraf reviewed Dec 18, 2023
View reviewed changes
src/mono/browser/wasm.proj Outdated Show resolved Hide resolved
src/mono/mono.proj
@@ -149,15 +149,15 @@
<!-- Sets up emscripten if you don't have the EMSDK_PATH env variable set -->
<Target Name="ProvisionEmscripten"
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: why is emscripten provisioning in mono.proj instead of wasm.proj?

Copy link
Member Author

Choose a reason for hiding this comment

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

@radekdoulik would know better. My guess is that it just happens to be there, and we can move it to wasm.proj .

src/mono/wasi/wasi.proj
<_FilesToCopy Include="$(MSBuildThisFileDirectory)..\wasm\runtime\runtime.h" DestinationFolder="$(NativeBinDir)include\wasm" />
<_FilesToCopy Include="$(MSBuildThisFileDirectory)..\wasm\runtime\pinvoke.h" DestinationFolder="$(NativeBinDir)include\wasm" />
<_FilesToCopy Include="$(WasiProjectRoot)mono-include\driver.h" DestinationFolder="$(NativeBinDir)include\wasm" />
<_FilesToCopy Include="$(BrowserProjectRoot)runtime\gc-common.h" DestinationFolder="$(NativeBinDir)include\wasm" />
Copy link
Member

Choose a reason for hiding this comment

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

Should we move these sources to common directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, in follow up PRs we can do the necessary moving around.

src/mono/wasm/build/WasmApp.LocalBuild.props Show resolved Hide resolved
src/mono/wasm/data/aot-tests/Directory.Build.props Show resolved Hide resolved
src/tests/FunctionalTests/WebAssembly/Directory.Build.props Outdated Show resolved Hide resolved
src/libraries/tests.proj Outdated Show resolved Hide resolved
src/libraries/sendtohelix-browser.targets
@@ -51,7 +49,7 @@
<EnableDefaultBuildHelixWorkItems>false</EnableDefaultBuildHelixWorkItems>

<!-- on unix CI has emscripten provisioned in $(EMSDK_PATH) as `/usr/local/emscripten`. -->
<EMSDK_PATH Condition="$([MSBuild]::IsOSPlatform('WINDOWS')) and '$(EMSDK_PATH)' == ''">$(RepoRoot)src\mono\wasm\emsdk\</EMSDK_PATH>
<EMSDK_PATH Condition="$([MSBuild]::IsOSPlatform('WINDOWS')) and '$(EMSDK_PATH)' == ''">$(RepoRoot)src\mono\browser\emsdk\</EMSDK_PATH>
Copy link
Member

Choose a reason for hiding this comment

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

Can we use ProvisionEmscriptenDir here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. We can move that property to the top level Directory.Build.props, and then use it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though the property put in D.B.props should be $(InTreeEmSdk), and that can be used in src/tests/FunctionalTests/WebAssembly/Directory.Build.props too.

@radical radical removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Dec 18, 2023
@radical
Copy link
Member Author

radical commented Dec 18, 2023

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Dec 18, 2023

/azp run runtime-wasm

@radical
Copy link
Member Author

radical commented Dec 18, 2023

/azp run runtime-wasm

@radical radical marked this pull request as ready for review December 18, 2023 19:32
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Dec 18, 2023

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Dec 18, 2023

Perf run for sanity check - https://dev.azure.com/dnceng/internal/_build/results?buildId=2338052

@radical
Copy link
Member Author

radical commented Dec 18, 2023

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Dec 18, 2023
Closed
Closed
@radical radical requested a review from maraf December 18, 2023 23:59
maraf
maraf approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@radical radical merged commit a36a860 into dotnet:main Dec 19, 2023
202 of 207 checks passed
akoeplinger added a commit that referenced this pull request Dec 27, 2023
@akoeplinger
Fix wasm Codespaces prebuild
7c2f3ac 
The makefile target was moved in #95940
@akoeplinger akoeplinger mentioned this pull request Dec 27, 2023
Merged
akoeplinger added a commit that referenced this pull request Dec 27, 2023
@akoeplinger
Fix wasm Codespaces prebuild (#96326)
18f3089 
The makefile target was moved in #95940
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Jan 8, 2024
@akoeplinger
[wasm] Fix a few more paths to in-tree emsdk
76bf4d7 
It was moved from src/mono/wasm to src/mono/browser in dotnet#95940
@akoeplinger akoeplinger mentioned this pull request Jan 8, 2024
Merged
akoeplinger added a commit that referenced this pull request Jan 8, 2024
@akoeplinger
[wasm] Fix a few more paths to in-tree emsdk (#96625)
425e74c 
It was moved from src/mono/wasm to src/mono/browser in #95940
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pavelsavara pavelsavara pavelsavara left review comments

@maraf maraf maraf approved these changes

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

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

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

@kg kg Awaiting requested review from kg

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

Assignees

@radical radical

Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@radical @pavelsavara @maraf