-
Couldn't load subscription status.
- Fork 5.2k
[browser] build browserhost in host.native subset #120298
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
Conversation
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
32ecbfa to
04232e2
Compare
3dfa996 to
2e8702e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
For Linux and Windows, we were hitting issues with linker incompatibilities and a ton of complexity because of the flags we pass around and the fact that we shuffle pieces between different machines in the PR builds. For wasm, we already have an established path of shipping static libs, so I'm not as concerned that we'll hit the same problem. If you're able to pre-link everything with the new host like how we do the singlefilehost, I'd recommend that. If you can't, then splitting it is fine. |
f86a2dc to
7070241
Compare
|
@jkoritzinsky I rebased this PR on top of option B) that was already merged into main branch. Now it should be easier to discuss option C) I don't insist it needs to be done this way, but it's much closer to what customers would run if they link the .a files on their dev environment using workload (LLVM/emcc). It also makes coreCLR CMake innner dev loop significantly faster. Emcc linking is slow and this way we only link corerun but not corehost in there. Effectively in half of the time. I also updated the top description. |
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.
Pull Request Overview
This PR restructures the browser (WASM) build for CoreCLR to separate the browserhost build into the host.native subset, aligning it with the standard build pattern. Previously, browserhost was built within the CoreCLR runtime subset, which violated the architecture where host components should be built separately.
Key Changes:
- Restored
host.nativesubset for CoreCLR browser builds - Removed browserhost from
clr.runtimesubset - Changed browserhost dependencies from CMake targets to static library file paths
- Introduced
CORERUN_LIBS_ONLYflag to reduce native library build scope when building for CoreCLR
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/CMakeLists.txt | Moved browser-specific build configuration from the runtime subset to the libs-native section and removed browserhost as a runtime dependency |
| src/native/corehost/browserhost/CMakeLists.txt | Changed from CMake target dependencies to explicit .a file dependencies; updated paths to use SHARED_LIB_DESTINATION and added new install targets |
| src/native/corehost/browserhost/sample/CMakeLists.txt | Updated sample paths from STATIC_LIB_DESTINATION to SHARED_LIB_DESTINATION |
| src/native/libs/CMakeLists.txt | Added conditional compilation guards using CORERUN_LIBS_ONLY to exclude non-essential libraries |
| src/native/libs/System.Native/CMakeLists.txt | Added dependency declarations for timezone data libraries |
| src/native/libs/Common/JavaScript/CMakeLists.txt | Added installation of package.json to sharedFramework |
| eng/native/configurepaths.cmake | Added CLR_ARTIFACTS_BIN_DIR variable definition |
| eng/native.props | Added CMake arguments for version and configuration information |
| eng/liveBuilds.targets | Updated artifact paths and file collections to reflect the new browserhost location in host.native subset |
| eng/Subsets.props | Added host.native to default CoreCLR browser subsets and reorganized libs.native subset definition |
We don't have to rush this PR, if we think we are going to return do B1 anyway. Before I close this, does any of you feel that C) with pre-generated |
I would avoid pre-generated wasm-pinvoke.c in git.
This should not be that bad if the generation is done in incremental build friendly fashion. If you are touching on that the incremental builds are slow, I think it is a general problem that we have been introducing by representing more dependencies correctly in the build system. It makes the incremental builds slower and slower. It has been very apparent in some of the inner loop experiences under libraries. I think the solution for that is to introduce gestures that one can use to skip the slow dependency checks, such as |
|
Well, my current incremental experience on windows.
|
|
It seems this is yet another time when the version.c is compiled unconditionally, triggering relinking. We should fix that. |
Side note: this is because of #119639. Adding |
# Conflicts: # eng/Subsets.props
|
My current understanding is that B1 would not be the final solution because of libs R2R and so we would do late linking .a files. Is there further feedback or we could merge this ? @jkotas @jkoritzinsky @radekdoulik |
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'm fine with this in its current state.
|
/ba-g CI timeout |
host.nativesubset back for coreCLR browserbrowserhostforclr.runtimesubsetbrowserhostdependent on relevant.afiles oflibs.native+clr.runtimesubsets, instead of CMake target andadd_subdirectoryeng/liveBuilds.targetsCORERUN_LIBS_ONLYto make libs build smaller when used from coreCLR CMakeCLR_ARTIFACTS_BIN_DIR,CMAKE_BUILD_LIBRARIES_CONFIGURATION,CMAKE_NET_CORE_APP_CURRENT_VERSION,CMAKE_BUILD_RUNTIME_CONFIGURATIONSHARED_LIB_DESTINATIONandSHARED_CLR_DESTINATIONin MSBuild and pass that instead.Contributes to #120206