[mono] new Sdk that selects mono runtime components by lambdageek · Pull Request #54432 · dotnet/runtime

lambdageek · 2021-06-18T21:01:15Z

Related to #54417

The contract is:

The runtime defines _MonoRuntimeAvailableComponents which is an item list like "hot_reload;diagnostic_tracing"
The workoad defines _MonoComponent which is some subset of the items that makes sense in the current configuration of the user app
The workload calls the _MonoSelectRuntimeComponents target
- The target returns _MonoRuntimeSelectedComponents (and _MonoRuntimeSelectedStubComponents if static linking) which are the names of the comopnents that will be available (or stubbed out, if static linking)
- The target returns _MonoRuntimeComponentLink which is a list of filenames (relative to the runtime pack native/ subdirectory) of the libraries that must be either copied (for dynamic loading) or statically linked (for static linking) with the runtime. The items also have some metadata such as IsStub, ComponentName and Linking
- The target also returns a _MonoRuntimeComponentDontLink list of filenames to exclude from linking for workloads which would rather take everything from native/ and just exclude what we tell them to exclude.

ghost · 2021-06-18T21:01:20Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #54417

The contract is:

The runtime defines _MonoRuntimeAvailableComponents which is an item list like "hot_reload;diagnostic_tracing"
The workoad defines _MonoComponent which is some subset of the times that makes sense in the current configuration of the user app
The workload calls the _MonoSelectRuntimeComponents target
- The target returns _MonoRuntimeSelectedComponents (and _MonoRuntimeSelectedStubComponents if static linking) which are the names of the comopnents that will be available (or stubbed out, if static linking)
- The target returns _MonoRuntimeComponentLink which is a list of filenames (relative to the runtime pack native/ subdirectory) of the libraries that must be either copied (for dynamic loading) or statically linked (for static linking) with the runtime. The items also have some metadata such as IsStub, ComponentName and Linking

TODO

Right now the component-manifest.props is generated from component-manifest.props.in when the nuget is built. this is wrong:
- the component-manifest.props file is RID-dependent for two reasons: whether we do static or dynamic linking of components depends on the target platform, and the file extensions also depend on the platform.
- the list of components is currently hardcoded in the pkgproj, but it should really come from the source of truth: src/mono/mono/component/CMakeLists.txt

Author:	lambdageek
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

lambdageek · 2021-06-23T15:37:23Z

Ok, added a workload manifest.

One thing that is surprising to me is that we have apparently an iossimulator-x86 runtime pack? Is that right? I didn't add the workload manifest for the Sdk for it. We need one, right?

lambdageek · 2021-06-23T15:40:06Z

Is this actually going to work? because of the indirection of going through pack IDs, only a build targeting, let's say... android-arm64 will load the Microsoft.NETCore.App.Runtime.Mono.android-arm64.Sdk nuget's Sdk.props and from there the component-manifest.props, right? And so that build will get the mono components for android-arm64.

steveisok · 2021-06-23T15:44:39Z

Is this actually going to work? because of the indirection of going through pack IDs, only a build targeting, let's say... android-arm64 will load the Microsoft.NETCore.App.Runtime.Mono.android-arm64.Sdk nuget's Sdk.props and from there the component-manifest.props, right? And so that build will get the mono components for android-arm64.

You need to add the right imports in https://github.com/dotnet/runtime/blob/4615e06d531d10cb0c4347173718ee30620fda9a/src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Manifest/WorkloadManifest.targets

lambdageek · 2021-06-23T15:55:05Z

Is this actually going to work? because of the indirection of going through pack IDs, only a build targeting, let's say... android-arm64 will load the Microsoft.NETCore.App.Runtime.Mono.android-arm64.Sdk nuget's Sdk.props and from there the component-manifest.props, right? And so that build will get the mono components for android-arm64.
You need to add the right imports in https://github.com/dotnet/runtime/blob/4615e06d531d10cb0c4347173718ee30620fda9a/src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Manifest/WorkloadManifest.targets

@steveisok Can I just do it like this? is TargetArchitecture defined?

 <ImportGroup>
<Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.$(TargetPlatformIdentifier)-$(TargetArchitecture).Sdk" />
<Import Project="Sdk.targets" Sdk="Microsoft.NETCore.App.Runtime.$(TargetPlatformIdentifier)-$(TargetArchitecture).Sdk" />
</ImportGroup>

lambdageek · 2021-06-23T18:22:33Z

Ok, so the plan here is twofold:

Make the mono-component.props include the host RID in the metadata. So we can include Sdk.props from multiple architectures without conflict.
Make a new nuget that just has the mono-components.targets in it that has the single target that is RID-independent. Update the target to pick from the components for the current target only.
Update the workload manifest for each workload to bring in all the props and the single target

marek-safar · 2021-06-24T10:49:18Z

@steveisok would be better to extract the plan to separate issue for tracking?

lambdageek · 2021-06-24T13:38:34Z

I updated the related issue #54417 with the notes from here. I also plan to update the design doc as part of this PR.

lewing · 2021-06-24T15:29:28Z

I think this could be simplified a lot if we handled it the way the Cross runtimes do? - which sound like roughly the plan from #54432 (comment)

steveisok · 2021-06-24T15:44:21Z

I think this could be simplified a lot if we handled it the way the Cross runtimes do? - which sound like roughly the plan from #54432 (comment)

Yep, the difference is that we also want to include a .targets file. We can only import that once, so it has to be in another pack.

lewing · 2021-06-24T15:46:46Z

I think this could be simplified a lot if we handled it the way the Cross runtimes do? - which sound like roughly the plan from #54432 (comment)
Yep, the difference is that we also want to include a .targets file. We can only import that once, so it has to be in another pack.

I think we could probably combine at least the runtime config and this into a single pack with targets that we enable appropriately. There isn't much need to have everything in a separate pack is there?

steveisok · 2021-06-24T15:53:27Z

No, there's not a real need to have them in a separate pack.

lambdageek · 2021-06-24T16:15:11Z

think we could probably combine at least the runtime config and this into a single pack with targets that we enable appropriately. There isn't much need to have everything in a separate pack is there?

Yea I think we should have a single Mono Targeting SDK nuget that has all tasks and targets that are portable across all host and target architectures. (At the moment it's the runtime config parser and this component selector)

steveisok

LGTM - nice work!

lewing

nit: could we s/component-manifest/RuntimeComponentManifest/ or something, adding yet another naming format is painful

instead of TargetsMobile. We want the build files (mono-components.json) in every mono runtime pack, not just on mobile targets

radical · 2021-07-07T17:14:49Z

+ {
+ var dict = JsonSerializer.Deserialize<Dictionary<string, string>>(ref reader, options);
+ if (dict == null)
+ return null!;


Should the uses of JsonSerializer.Deserialize* also catch JsonException?

I don't think so - we want it to propagate

Ok, then we should catch this up the chain somewhere, so we can log it as an error with the filename.

Co-authored-by: Ankit Jain <radical@gmail.com>

radical · 2021-07-07T18:10:01Z

Don't wanna block you, but would it be possible to add a sample for this? In future we will be able to test this, at least on wasm. But for now, a sample would help validate it. Not sure if that makes sense though!

from MonoRuntimeComponentsReadManifestTask

Build doesn't like it for some reason (probably net472)

lambdageek · 2021-07-07T18:45:23Z

Don't wanna block you, but would it be possible to add a sample for this? In future we will be able to test this, at least on wasm. But for now, a sample would help validate it. Not sure if that makes sense though!

@radical
It's hard to test the workloads end to end. Here's a gist of what I use for testing (but as you noticed, I didn't test every time). Not sure where's a good place to check this in. https://gist.github.com/lambdageek/7bb9672fa0a95bb232926aa7630962f4

Also improve nullability a bit by making the properties immutable

radical · 2021-07-07T18:54:57Z

It's hard to test the workloads end to end. Here's a gist of what I use for testing (but as you noticed, I didn't test every time). Not sure where's a good place to check this in. gist.github.com/lambdageek/7bb9672fa0a95bb232926aa7630962f4

Makes sense. I have a PR open that will add some E2E(almost) testing in runtime, and I can add a test for this after that.

We get nice error messages now like ``` src/mono/nuget/Microsoft.NET.Runtime.MonoTargets.Sdk/Sdk/RuntimeComponentManifest.targets(8,5): error : Failed to deserialize json from file 'artifacts/bin/mono/iOSSimulator.x64.Release/build/RuntimeComponentManifest.json', JSON Path: $.items._MonoRuntimeAvailableComponents[2], Line: 14, Position: 1 [component-manifest.sample.proj] src/mono/nuget/Microsoft.NET.Runtime.MonoTargets.Sdk/Sdk/RuntimeComponentManifest.targets(8,5): error : JsonException: The JSON value could not be converted to System.Collections.Generic.Dictionary`2[System.String,System.String]. Path: $.identity | LineNumber: 0 | BytePositionInLine: 16. Path: $.items._MonoRuntimeAvailableComponents[2] | LineNumber: 14 | BytePositionInLine: 1. [component-manifest.sample.proj] src/mono/nuget/Microsoft.NET.Runtime.MonoTargets.Sdk/Sdk/RuntimeComponentManifest.targets(8,5): error : InvalidOperationException: Cannot get the value of a token type 'Number' as a string. [component-manifest.sample.proj] src/mono/nuget/Microsoft.NET.Runtime.MonoTargets.Sdk/Sdk/RuntimeComponentManifest.targets(8,5): error : [component-manifest.sample.proj] ```

ghost · 2021-07-07T19:55:56Z

Hello @lambdageek!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

radical · 2021-07-07T19:56:23Z

 return new JsonModelItem(identity, metadata: idict);
 default:
- throw new Exception();
+ throw new NotSupportedException();


token type in the message might be useful for debugging

The JsonSerializer catches and decorates NotSupportedException with location and type info https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-converters-how-to?pivots=dotnet-5-0#notsupportedexception

ghost added the area-System.Net label Jun 18, 2021

lambdageek added area-Build-mono and removed area-System.Net labels Jun 18, 2021

This comment has been minimized.
Sign in to view

lambdageek marked this pull request as ready for review June 23, 2021 15:37

lambdageek requested a review from marek-safar as a code owner June 23, 2021 15:37

lambdageek requested review from akoeplinger, directhex and steveisok June 23, 2021 15:37

lambdageek changed the title ~~[draft] new Sdk that selects mono runtime components~~[mono] new Sdk that selects mono runtime componentsJun 23, 2021

lambdageek force-pushed the mono-runtime-component-manifest branch from fd2c306 to 4492230Compare June 25, 2021 03:33

This comment has been minimized.
Sign in to view

marek-safar reviewed Jun 25, 2021
View reviewed changes

Comment threadsrc/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Manifest/WorkloadManifest.json.in Outdated

marek-safar requested a review from radical June 25, 2021 12:27

steveisok requested a review from lewing June 25, 2021 13:48

Fix some copy-paste nits
756e5a6

lambdageek force-pushed the mono-runtime-component-manifest branch from c09d837 to 756e5a6Compare July 7, 2021 14:47

steveisok reviewed Jul 7, 2021
View reviewed changes

Comment threadsrc/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Runtime.props Outdated

steveisok approved these changes Jul 7, 2021
View reviewed changes

lewing reviewed Jul 7, 2021
View reviewed changes

Comment threadsrc/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Manifest/WorkloadManifest.json.in Outdated

Use RuntimeFlavor==mono for runtime pack build directory
372df69
instead of TargetsMobile. We want the build files (mono-components.json) in every mono runtime pack, not just on mobile targets

radical reviewed Jul 7, 2021
View reviewed changes

lambdageekand others added 6 commits July 7, 2021 13:48

Apply suggestions from code review
2cc01b6
Co-authored-by: Ankit Jain <radical@gmail.com>

rename component-manifest to RuntimeComponentManifest
065a2bb

fixup nullability annotations
6957bcc

fix whitespace
dae9aeb

fix formatting
d5e6f0e

Misc fixes to JsonToItemsTaskFactory
eb0d0d4

lambdageek added 3 commits July 7, 2021 14:32

Rename MonoRuntimeComponentManifestReadTask
4874bd4
from MonoRuntimeComponentsReadManifestTask

undo nullability annotation
011e7ab
Build doesn't like it for some reason (probably net472)

fix incorrect task parameter name
c332f7b

Remove Identity metadata from dictionary at json parsing time
7d6ca6d
Also improve nullability a bit by making the properties immutable

lambdageek added 2 commits July 7, 2021 15:10

Throw correct json deserializer exceptions
174065e

lambdageek added the auto-merge label Jul 7, 2021

radical reviewed Jul 7, 2021
View reviewed changes

fixup comments
6213067

ghost merged commit 893739f into dotnet:mainJul 8, 2021

rolfbjarne mentioned this pull request Jul 12, 2021
Add support for consuming the selected Mono components correctly dotnet/macios#12100
Closed

lewing mentioned this pull request Jul 14, 2021
[browser-wasm] Support Mono runtime components in workload #54565
Closed

ghost locked as resolved and limited conversation to collaborators Aug 7, 2021

This pull request was closed.

Uh oh!

Conversation

lambdageek commented Jun 18, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 18, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lambdageek commented Jun 23, 2021

Uh oh!

lambdageek commented Jun 23, 2021

Uh oh!

steveisok commented Jun 23, 2021

Uh oh!

lambdageek commented Jun 23, 2021

Uh oh!

lambdageek commented Jun 23, 2021

Uh oh!

marek-safar commented Jun 24, 2021

Uh oh!

lambdageek commented Jun 24, 2021

Uh oh!

lewing commented Jun 24, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveisok commented Jun 24, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lewing commented Jun 24, 2021

Uh oh!

steveisok commented Jun 24, 2021

Uh oh!

lambdageek commented Jun 24, 2021

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

steveisok left a comment

Choose a reason for hiding this comment

Uh oh!

lewing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

radicalJul 7, 2021

Choose a reason for hiding this comment

Uh oh!

lambdageekJul 7, 2021

Choose a reason for hiding this comment

Uh oh!

radicalJul 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

radical commented Jul 7, 2021

Uh oh!

lambdageek commented Jul 7, 2021

Uh oh!

radical commented Jul 7, 2021

Uh oh!

ghost commented Jul 7, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

radicalJul 7, 2021

Choose a reason for hiding this comment

Uh oh!

lambdageekJul 7, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

lambdageek commented Jun 18, 2021•
edited
Loading

lewing commented Jun 24, 2021•
edited
Loading

steveisok commented Jun 24, 2021•
edited
Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.