--use-current-runtime semantics

[tmds]

#14093 adds a new option that makes the sdk use an appropriate rid for publishing.

There was an on-going discussion whether the appropriate rid is the portable rid, machine rid, or whether both options should be added, see #14093 (comment).

cc @eerhardt @tmds, @dagood, @am11, @dsplaisted, @janvorli, @marcpopMSFT

[tmds]

I think portable rid is the main use-case.
machine rid is niche, and it may be even desired that the user needs to specify the rid in this case.

I'd prefer the new option uses the portable rid on Microsoft and source-build sdk, and we update the docs/implementation/option name accordingly.

[am11]

The place where source-build is overriding NETCoreSdkRuntimeIdentifier from e.g. linux-x64 to rhel.6-x64, and after the Microsoft.NET.RuntimeIdentifierInference.targets has ran, source-build can then also override the RuntimeIdentifier value set by that targets as:

  <PropertyGroup Condition="'$(UseCurrentRuntimeIdentifier)' == 'true'">
    <RuntimeIdentifier>$(NameOfTheSourceBuildSpecificVariableWhichHoldsPortableRID)</RuntimeIdentifier>
  </PropertyGroup>

I believe this will be enough to align the source-build and Microsoft SDKs behaviors in a way that both are using portable RID, without changing anything else in this repo (including public docs that can continue to stay impartial about the implementation detail and avoiding terms like portable RID)?

[dagood]

The place where source-build is overriding NETCoreSdkRuntimeIdentifier from e.g. linux-x64 to rhel.6-x64, and after the Microsoft.NET.RuntimeIdentifierInference.targets has ran

This isn't how this works--when dotnet/installer is built by source-build, dotnet/installer generates Microsoft.NETCoreSdk.BundledVersions.props and writes the non-portable RID as the value of the NETCoreSdkRuntimeIdentifier property. This is the correct value, because the SDK does bundle a non-portable runtime/shared framework and a non-portable AppHost pack when built for a non-portable RID.

The rest of your suggestion makes sense, but it should be implemented in dotnet/sdk (and dotnet/installer if more values need to be preserved from build time => runtime).

[dsplaisted]

It sounds like we should update Microsoft.NETCoreSdk.BundledVersions.props to include a new property such as NETCoreSdkPortableRuntimeIdentifier. This would be the same as the existing NETCoreSdkRuntimeIdentifier for normal builds of the SDK, but would be different for source-build.

Then we can update the UseCurrentRuntimeIdentifier property logic to use the new NETCoreSdkPortableRuntimeIdentifier.

I don't know much about how source build works though, will it be simple enough to write the portable RID to the new property in the dotnet-installer source-build build?

[dagood]

Yeah, dotnet/installer uses $(ProductMonikerRid) here: https://github.com/dotnet/installer/blob/f8eb583d40b39ff59b52adff3cbbfd1c4e9a7d8b/src/redist/targets/GenerateBundledVersions.targets#L202

ProductMonikerRid is put together here: https://github.com/dotnet/installer/blob/efe7c8e566c4c06411d028d8d565cb5f6ef7c3e4/src/redist/targets/GetRuntimeInformation.targets#L24-L27.

Source-build passes in a non-portable OSName as an MSBuild global property (if building non-portable): https://github.com/dotnet/source-build/blob/2b1abb23997ef7cd23182455e0c6566e205e43d0/repos/core-sdk.proj#L13

And I think Architecture comes from the default machine-based detection here: https://github.com/dotnet/installer/blob/17b5d95d8be3f634e0547dfdde6cc09d29a95ed0/Directory.Build.props#L11-L13

The dotnet/installer repo could just keep the calculations here but call it PortableOSName: https://github.com/dotnet/installer/blob/efe7c8e566c4c06411d028d8d565cb5f6ef7c3e4/src/redist/targets/GetRuntimeInformation.targets#L7-L11

And then set OSName to PortableOSName only if no OSName is specified, something like this:

<PropertyGroup>
  <IsLinux Condition = " $([MSBuild]::IsOSPlatform('LINUX')) ">True</IsLinux>
  <PortableOSName Condition=" '$(PortableOSName)' == '' AND $([MSBuild]::IsOSPlatform('WINDOWS')) ">win</PortableOSName>
  <PortableOSName Condition=" '$(PortableOSName)' == '' AND $([MSBuild]::IsOSPlatform('OSX')) ">osx</PortableOSName>
  <PortableOSName Condition=" '$(PortableOSName)' == '' AND $([MSBuild]::IsOSPlatform('FREEBSD')) ">freebsd</PortableOSName>
  <PortableOSName Condition=" '$(PortableOSName)' == '' AND '$(IsLinux)' == 'True' ">linux</PortableOSName>

  <OSName Condition=" '$(OSName)' == '' AND '$(PortableOSName)' != '' ">$(PortableOSName)</OSName>

  <Rid Condition=" '$(Rid)' == '' ">$(OSName)-$(Architecture)</Rid>

  <ProductMonikerRid Condition=" '$(ProductMonikerRid)' == '' ">$(OSName)-$(Architecture)</ProductMonikerRid>
  <ProductMonikerPortableRid Condition=" '$(ProductMonikerPortableRid)' == '' ">$(PortableOSName)-$(Architecture)</ProductMonikerPortableRid>
</PropertyGroup>

So back in GenerateBundledVersions.targets:

    <NETCoreSdkRuntimeIdentifier>$(ProductMonikerRid)</NETCoreSdkRuntimeIdentifier>
    <NETCoreSdkPortableRuntimeIdentifier>$(ProductMonikerPortableRid)</NETCoreSdkPortableRuntimeIdentifier>

Since source-build will be moving into the repos with the arcade-powered source-build plan, this logic will be in dotnet/installer either way, but other than that we can calculate the portable RID wherever it makes the most sense.

[dsplaisted]

Hi folks,

I've sent a PR which includes the first step of creating a property that holds the portable RuntimeIdentifier for the SDK: dotnet/installer#9106

[marcpopMSFT]

Are there still open issues under discussion here?

[tmds]

afaik we still need to make a change so --use-current-runtime means: use portable rid in a source-built sdk.

I've sent a PR which includes the first step of creating a property that holds the portable RuntimeIdentifier for the SDK: dotnet/installer#9106

We can use the information from this property.

I'd like this to work for 6.0. I'll look into making a PR against main next week.

We can see how we get it into the source-build sdk. Either using a source-build patch, by adding it to sdk 6.0.100 branch (probably too late), or include it in a later 6.0 sdk version as a fix.

cc @dseefeld @omajid