Skip to content

[wasm] Remove hardcoded references to net8.0 from WasmApp*targets #89699

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
Merged
radical merged 14 commits into from
Aug 3, 2023
Merged

[wasm] Remove hardcoded references to net8.0 from WasmApp*targets #89699

radical merged 14 commits into from
Aug 3, 2023

Conversation

@radical
Copy link
Member

@radical radical commented Jul 31, 2023 •
edited
Loading

Look for System.Runtime.dll to find the runtime pack directory with the assemblies, so we don't need to figure out the tfm for the path.

  • Cannot use System.Private.CoreLib.dll because that is in native/ instead of lib/net8.0
  • Cannot use RuntimePackAsset because in many places that item isn't available.
    • AOT on helix where we run only the wasm targets on the proxy project.
    • Runtime tests which currently dump all the assemblies, and other bits into a single directory, not following the directory structure of a runtime pack. This also means the RuntimePackAsset cannot easily be synthesized.

Fixes #79109 .

@radical radical added the arch-wasm WebAssembly architecture label Jul 31, 2023
@ghost ghost added the area-Build-mono label Jul 31, 2023
@ghost ghost assigned radical Jul 31, 2023
@ghost
Copy link

ghost commented Jul 31, 2023

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

Issue Details

Fixes #79109 .

Author: radical
Assignees: -
Labels:

arch-wasm
Milestone: -

@radical radical mentioned this pull request Jul 31, 2023
Merged
@radical radical marked this pull request as draft July 31, 2023 01:16
@build-analysis build-analysis bot mentioned this pull request Jul 31, 2023
Closed
ViktorHofer
ViktorHofer reviewed Jul 31, 2023
View reviewed changes
ViktorHofer
ViktorHofer reviewed Jul 31, 2023
View reviewed changes
@radical radical marked this pull request as ready for review July 31, 2023 23:40
ViktorHofer
ViktorHofer reviewed Aug 1, 2023
View reviewed changes
ViktorHofer
ViktorHofer reviewed Aug 1, 2023
View reviewed changes
src/mono/wasm/build/WasmApp.Native.targets Outdated
Copy link
Member

@ViktorHofer ViktorHofer Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why System.IO? System.IO is a pure shim assembly which might get trimmed out in certain scenarios.

Copy link
Member Author

@radical radical Aug 1, 2023

Choose a reason for hiding this comment

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

Trimmed from the runtime pack?

They are in different paths:
./Microsoft.NETCore.App.Runtime.Mono.browser-wasm/8.0.0-dev/runtimes/browser-wasm/native/System.Private.CoreLib.dll
./Microsoft.NETCore.App.Runtime.Mono.browser-wasm/8.0.0-dev/runtimes/browser-wasm/lib/net8.0/System.IO.dll

.. and we need the latter one.

Copy link
Member

@ViktorHofer ViktorHofer Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"C:\Users\vihofer.nuget\packages\microsoft.netcore.app.runtime.win-x64\8.0.0-rc.1.23381.3\runtimes\win-x64\lib\net8.0\System.Private.CoreLib.dll"

Why is the mono runtime pack different? The CoreCLR one has System.Private.CoreLib in the lib directory (as it's a managed library). cc @akoeplinger

Trimmed from the runtime pack?

Not from the runtime pack directly, but the linker trims out of some shim assemblies afaik. I wasn't sure if this code path would run before or after but I guess it's before. You could consider using an assembly that is more "core" to the .NETCoreApp shared framework, i.e. System.Runtime but I'm also ok with the current state.

Copy link
Member Author

@radical radical Aug 1, 2023

Choose a reason for hiding this comment

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

Looks like this fails for AOT/helix where we create a proxy project, and run only the wasm part of the build. And that doesn't have RuntimePackAsset, because none of the targets related to that were run.

I will try to populate RuntimePackAsset in the proxy project, but that will be bit of a hack.

ViktorHofer
ViktorHofer previously approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

If that works, looks good. Please double check that _MscorlibPath, _SystemRuntimePath and _WasmAOTSearchPaths include the correct values.

Thank you

@radical
Copy link
Member Author

radical commented Aug 1, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 1, 2023

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Aug 1, 2023
Closed
Closed
@radical
Copy link
Member Author

radical commented Aug 1, 2023

AOT failure might be relevant. Investigating.

@radical radical added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 1, 2023
@radical
Copy link
Member Author

radical commented Aug 1, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 1, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Aug 2, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 2, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Aug 2, 2023

I had to change from using RuntimePackAsset, because it is not available in many setups we test like AOT on helix.
This still does not fix the runtime tests though which seem to just dump all the assemblies into a native folder! I'm working on fixing that.

@radical
Copy link
Member Author

radical commented Aug 2, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 2, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@radical radical requested a review from ViktorHofer August 2, 2023 20:23
@radical
Copy link
Member Author

radical commented Aug 3, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 3, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@ViktorHofer
Copy link
Member

ViktorHofer commented Aug 3, 2023

@radical you added unrelated commits, didn't you?

@radical
Copy link
Member Author

radical commented Aug 3, 2023

@radical you added unrelated commits, didn't you?

The merge seems messed up a bit, so git log shows extra commits. But the only actual changes here are the relevant ones. I was avoiding cleaning this up to not mess up reviewing on github (it won't show "changes since last review" if you force-push), but I'll do that now.

radical added 14 commits August 3, 2023 13:57
@radical
[wasm] Remove hardcoded references to net8.0 from WasmApp*targets
6167230 
Fixes dotnet#79109 .
@radical
Address feedback from @ViktorHofer
38d74d9
@radical
address review feedback
a358520
@radical
Use System.Runtime.dll instead of System.IO.dll
33a6aa4
@radical
Fix local run with BuildAOTTestsOnHelix=true, to get the correct task…
b0cd025 
…s config
@radical
Add $(MicrosoftNetCoreAppRuntimePackRidLibTfmDir)
bc07a51
@radical
WasmApp.LocalBuild.props: populate a synthetic RuntimePackAsset item,…
ba1dfdc 
… as native build depends on that
@radical
WasmApp.targets: Fix the logic to find runtime pack path with the
1974848 
.. assemblies. `RuntimePackAsset` can't be reliably used because it is
not always available in the various test setups used. Instead, find
`System.Runtime.dll` in the runtime pack path, which is already known.
@radical
WasmApp.targets - error check before using the item
d997f9d
@radical
Fix runtime tests, by overriding the path to the assemblies
685a39f
@radical
Fix runtime tests, by overriding the path to the assemblies
cb63377
@radical
Apply the same changes to wasi targets also
1af5539
@radical
fix
5119f43
@radical
fix
6106560
@radical radical force-pushed the wasm-hardcoded-tfm branch from f177757 to 6106560 Compare August 3, 2023 17:57
@radical
Copy link
Member Author

radical commented Aug 3, 2023

Force pushed with the rebased commits.

ViktorHofer
ViktorHofer approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Thanks

@radical radical merged commit 2ab3874 into dotnet:main Aug 3, 2023
@radical radical deleted the wasm-hardcoded-tfm branch August 3, 2023 22:49
@build-analysis build-analysis bot mentioned this pull request Aug 4, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@lewing lewing Awaiting requested review from lewing

@steveisok steveisok Awaiting requested review from steveisok

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.

Remove hardcoded TFM in wasm build files

3 participants

@radical @pavelsavara @ViktorHofer