Skip to content

Support labels for runtime packs #11824

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
merged 7 commits into from
Jun 20, 2020
Merged

Support labels for runtime packs #11824

merged 7 commits into from
Jun 20, 2020

Conversation

dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented May 30, 2020 •
edited
Loading

This PR will:

  • Allow available runtime packs to be specified in KnownRuntimePack items, separately from KnownFrameworkReference items
  • Allow a set of runtime labels to be specified on a FrameworkReference via RuntimePackLabels metadata
  • Require that the resolved runtime pack has the same set of labels specified in the FrameworkReference
  • In the test framework, refactor BuildCommand and RestoreCommand so they can be used with less boilerplate code

This will allow separate runtime packs to be specified for Mono, and for projects to opt in to using the Mono runtime packs. It could also be used for other options, such as AOT policy. See dotnet/runtime#34193.

A KnownRuntimePack item has much of the same information as a KnownFrameworkReference. It can look like this:

    <KnownRuntimePack Include="Microsoft.NETCore.App"
                      TargetFramework="net5.0"
                      RuntimeFrameworkName="Microsoft.NETCore.App"
                      DefaultRuntimeFrameworkVersion="5.0.0-preview1"
                      LatestRuntimeFrameworkVersion="5.0.0-preview1.1"
                      RuntimePackNamePatterns="Microsoft.NETCore.App.Runtime.Mono.**RID**"
                      RuntimePackRuntimeIdentifiers="linux-arm;linux-arm64;linux-musl-arm64;linux-musl-x64;linux-x64;osx-x64;rhel.6-x64;tizen.4.0.0-armel;tizen.5.0.0-armel;win-arm;win-arm64;win-x64;win-x86;ios-arm64;ios-arm;ios-x64;ios-x86;tvos-arm64;tvos-x64;android-arm64;android-arm;android-x64;android-x86;browser-wasm"
                      IsTrimmable="true"
                      RuntimePackLabels="Mono"
                      />

This would be selected when the FrameworkReference has metadata that specifies RuntimePackLabels="Mono". This can be done with the following:

<FrameworkReference Update="Microsoft.NETCore.App"
                     RuntimePackLabels="Mono" />

Note that this isn't the primary method by which end-users should opt into a specific runtime pack, but rather the underlying mechanism we would use to implement user-facing properties such as UseMonoRuntime.

@dsplaisted dsplaisted force-pushed the runtime-pack-types branch from f4d83b6 to d4f7c6b Compare June 1, 2020 18:20
@dsplaisted
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@dsplaisted dsplaisted force-pushed the runtime-pack-types branch from d4f7c6b to 262fa48 Compare June 1, 2020 19:56
@dsplaisted
Copy link
Member Author

Also related to #11227

@dsplaisted dsplaisted force-pushed the runtime-pack-types branch 2 times, most recently from 3158106 to 5f1cab0 Compare June 18, 2020 18:26
@dsplaisted dsplaisted force-pushed the runtime-pack-types branch from 5f1cab0 to 96b6408 Compare June 18, 2020 19:40
@dsplaisted dsplaisted marked this pull request as ready for review June 18, 2020 22:27
@dsplaisted dsplaisted requested review from sfoslund, wli3 and a team June 19, 2020 00:37
wli3
wli3 approved these changes Jun 20, 2020
View reviewed changes
src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

// The ID of the targeting pack NuGet package to reference
public string TargetingPackName => _item.GetMetadata("TargetingPackName");
public string TargetingPackVersion => _item.GetMetadata("TargetingPackVersion");
public string TargetingPackFormat => _item.GetMetadata("TargetingPackFormat");

//public string RuntimePackNamePatterns => _item.GetMetadata("RuntimePackNamePatterns");

Copy link

Choose a reason for hiding this comment

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

clean up

src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs
public string Name => _item.ItemSpec;

//// The framework name to write to the runtimeconfig file (and the name of the folder under dotnet/shared)
//public string RuntimeFrameworkName => _item.GetMetadata(MetadataKeys.RuntimeFrameworkName);
Copy link

Choose a reason for hiding this comment

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

clean up

@wli3 wli3 mentioned this pull request Jun 20, 2020
Closed
@wli3
Copy link

wli3 commented Jun 20, 2020

I need this PR to be in to continue cswinrt work or I will have a really bad merge conflict. I will address my comments with cswinrt PR (or a separate PR).

@wli3 wli3 merged commit 4112eb3 into dotnet:master Jun 20, 2020
This was referenced Jun 22, 2020
Merged
Merged
@sfoslund
Copy link
Member

@wli3 @dsplaisted what's the motivation for filtering out profiles like WindowsForms and WPF? It looks like this caused the bug in the SDK-> installer insertions because WindowsForms has an extra RID in its RuntimePackRuntimeIdentifiers metadata (win-arm64) that gets lost with the new logic.

If we change matchingRuntimePacks[0] to knownFrameworkReference.ToKnownRuntimePack() here it fixes things because we get WindowsForms, including the extra RID, instead of the runtime pack that only has win-x86 and win-x64. Although that essentially side steps a lot of the new logic.

@dsplaisted
Copy link
Member Author

I need this PR to be in to continue cswinrt work or I will have a really bad merge conflict. I will address my comments with cswinrt PR (or a separate PR).

Thanks @wli3!

@dsplaisted
Copy link
Member Author

@wli3 @dsplaisted what's the motivation for filtering out profiles like WindowsForms and WPF? It looks like this caused the bug in the SDK-> installer insertions because WindowsForms has an extra RID in its RuntimePackRuntimeIdentifiers metadata (win-arm64) that gets lost with the new logic.

If we change matchingRuntimePacks[0] to knownFrameworkReference.ToKnownRuntimePack() here it fixes things because we get WindowsForms, including the extra RID, instead of the runtime pack that only has win-x86 and win-x64. Although that essentially side steps a lot of the new logic.

@sfoslund The motivation was so that you would only have to define a runtime pack once and it would apply to all possible profiles. I think it also simplified the implementation.

I hadn't considered Windows Forms supporting different RIDs than WPF, and I'm not sure exactly what to do to fix it. I would lean towards some kind of special case fix, since I think WPF is supposed to support ARM64 eventually.

@sfoslund sfoslund mentioned this pull request Jun 25, 2020
Merged
rainersigwald pushed a commit that referenced this pull request Jul 20, 2020
@mmitche
Merge pull request #11824 from dotnet/merges/release/2.1.5xx-to-relea…
f769321 
…se/2.1.6xx

Merge release/2.1.5xx to release/2.1.6xx
@dsplaisted dsplaisted mentioned this pull request Aug 20, 2020
Closed
@dsplaisted dsplaisted mentioned this pull request Oct 30, 2020
Closed
@akoeplinger akoeplinger mentioned this pull request Jan 28, 2021
Closed
@akoeplinger akoeplinger mentioned this pull request Apr 13, 2021
Merged
marcpopMSFT pushed a commit to dotnet/installer that referenced this pull request Apr 13, 2021
@akoeplinger
Add KnownRuntimePack entries for Mono-based runtime packs
b75e498 
We need this now that we changed the name of the Mono-based runtime packs.

See dotnet/sdk#11824 for the design.

Fixes dotnet/sdk#11227
@sbomer sbomer mentioned this pull request Jan 31, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wli3 wli3 wli3 approved these changes

@sfoslund sfoslund Awaiting requested review from sfoslund

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dsplaisted @wli3 @sfoslund