Consume portable RID graph from runtime #17760

ViktorHofer · 2023-11-10T18:40:56Z

Consume portable RID graph from runtime (subsequent PR will delete it from sdk).
Use the portable RID graph in source build projects
Delete the mutate RID graph and invocation as that's already handled directly in runtime.

1. Consume portable RID graph from runtime (subsequent PR will delete it from sdk). 2. Use the portable RID graph in source build projects 3. Delete the mutate RID graph and invocation as that's already handled directly in runtime.

ViktorHofer · 2023-11-10T18:42:04Z

src/redist/targets/GenerateBundledVersions.targets

    <GenerateSdkRuntimeIdentifierChain
          RuntimeIdentifier="$(PortableProductMonikerRid)"
-          RuntimeIdentifierGraphPath="$(SdkOutputDirectory)RuntimeIdentifierGraph.json"
+          RuntimeIdentifierGraphPath="$(SdkOutputDirectory)PortableRuntimeIdentifierGraph.json"


I have absolutely no idea what that chain file is about but it should still work as expected with the portable RID graph. In general, our product build shouldn't use the legacy RID graph anymore which only exists to make the back compat switch work.

This chain file contains an ordered list of rids that are usable with the rid being built.

On the 8.0 rc2 I have installed on Fedora it contains:

linux-x64 linux unix-x64 unix any

I'm slightly surprised it doesn't include the source-build non-portable rid.
The CLI seems to use it for some things.

Contributes to #35750 Related: dotnet/installer#17760

MichaelSimons · 2023-11-13T14:16:50Z

src/SourceBuild/content/repo-projects/runtime.proj

  <Target Name="SetOutputList" AfterTargets="Package" BeforeTargets="GatherBuiltPackages">
    <ItemGroup>
      <PackagesOutputList Include="$(ShippingPackagesOutput)" />
      <PackagesOutputList Include="$(NonShippingPackagesOutput)" />
    </ItemGroup>
  </Target>

-  <Target Name="UpdateRuntimeGraph"


Nice to see this source-build specialization removed.

MichaelSimons · 2023-11-14T16:59:46Z

smoke-tests surfaced some rid related issues:

System.InvalidOperationException : Failed to execute /vmr/test/Microsoft.DotNet.SourceBuild.SmokeTests/bin/Release/net9.0/.dotnet/dotnet publish --self-contained true -r linux-x64 /p:PublishTrimmed=true /p:PublishReadyToRun=true /bl:/vmr/test/Microsoft.DotNet.SourceBuild.SmokeTests/bin/Release/net9.0/logs/BasicScenarioTests_Console_CSharp-publish-self-contained-linux-x64-trimmed-R2R.binlog -o /vmr/test/Microsoft.DotNet.SourceBuild.SmokeTests/bin/Release/net9.0/projects-202311101933037072/BasicScenarioTests_Console_CSharp/bin/publish
Exit code: 1
MSBuild version 17.9.0-preview-23557-02+5d1509792 for .NET
Determining projects to restore...
/vmr/test/Microsoft.DotNet.SourceBuild.SmokeTests/bin/Release/net9.0/.dotnet/sdk/9.0.100-alpha.1.23560.1/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(90,5): error NETSDK1094: Unable to optimize assemblies for performance: a valid runtime package was not found. Either set the PublishReadyToRun property to false, or use a supported runtime identifier when publishing. When targeting .NET 6 or higher, make sure to restore packages with the PublishReadyToRun property set to true. [/vmr/test/Microsoft.DotNet.SourceBuild.SmokeTests/bin/Release/net9.0/projects-202311101933037072/BasicScenarioTests_Console_CSharp/BasicScenarioTests_Console_CSharp.csproj]

ViktorHofer · 2023-11-14T17:11:42Z

Yes, I hope that @tmds can help me understand the failures: 9ce6a45#r132417371

tmds · 2023-11-15T10:55:04Z

src/redist/targets/GenerateBundledVersions.targets

@@ -1195,9 +1195,13 @@ Copyright (c) .NET Foundation. All rights reserved.
          DestinationFiles="$(SdkOutputDirectory)RuntimeIdentifierGraph.json"
          SkipUnchangedFiles="true"/>

+    <Copy SourceFiles="$(NuGetPackageRoot)/microsoft.netcore.platforms/$(_NETCorePlatformsPackageVersion)/PortableRuntimeIdentifierGraph.json"


For my understanding:
Is this copying from a package that is produced by the runtime repo build?
Did we not have to copy this previously because it came with the sdk repo build output?

Exactly. The package is produced in runtime. The portable RID graph was previously checked into the sdk directly. We want to remove that extra copy now and use the package from runtime as the source of truth.

tmds · 2023-11-15T11:11:31Z

Yes, I hope that @tmds can help me understand the failures: 9ce6a45#r132417371

I think there's an issue mapping the non-portable source-built rid (NETCoreSdkRuntimeIdentifier) to a portable rid (for Crossgen2) using the rid graph (RuntimeIdentifierGraphPath):

    <ProcessFrameworkReferences FrameworkReferences="@(FrameworkReference)"
...
                                RuntimeGraphPath="$(RuntimeIdentifierGraphPath)"
...
                                NETCoreSdkRuntimeIdentifier="$(NETCoreSdkRuntimeIdentifier)"
...

That would mean that the non-portable rid was not added to the RuntimeIdentifierGraphPath gets used here.

ViktorHofer · 2023-11-15T14:59:56Z

@tmds do you know where that file is coming from? I can't find it checked-in anywhere. 9ce6a45#diff-de198c84af9d2e1870737bbc063dd42615618c702cc0a1f4c41b40fc11a16e59R78

tmds · 2023-11-15T15:48:33Z

@tmds do you know where that file is coming from? I can't find it checked-in anywhere. 9ce6a45#diff-de198c84af9d2e1870737bbc063dd42615618c702cc0a1f4c41b40fc11a16e59R78

In Microsoft.NET.Sdk.targets, RuntimeIdentifierGraphPath defaults to PortableRuntimeIdentifierGraph.json (unless UseRidGraph is set, which the smoke tests don't do).

Probably the changes cause to use the original PortableRuntimeIdentifierGraph.json instead of the one that gets patched with the source-build rid during the runtime build.

ViktorHofer · 2023-11-15T16:11:59Z

Right. But do you know where this path is: $(ProjectDirectory)pkg/Microsoft.NETCore.Platforms/runtime.json? Where is this pkg folder?

tmds · 2023-11-15T16:21:54Z

Right. But do you know where this path is: $(ProjectDirectory)pkg/Microsoft.NETCore.Platforms/runtime.json? Where is this pkg folder?

installer/src/SourceBuild/content/repo-projects/runtime.proj

Lines 64 to 74 in dccd713

    
           <Target Name="UpdateRuntimeGraph" 
        
                   BeforeTargets="Build" 
        
                   Condition="'$(_IsBootstrapping)' == 'true'"> 
        
             <PropertyGroup> 
        
               <RuntimeJsonFile>$(ProjectDirectory)pkg/Microsoft.NETCore.Platforms/runtime.json</RuntimeJsonFile> 
        
             </PropertyGroup> 
        
             <Message Importance="High" Text="Adding rid, $(TargetRid), to $(RuntimeJsonFile)" /> 
        
             <AddRidToRuntimeJson RuntimeJson="$(RuntimeJsonFile)" 
        
                                  Rid="$(TargetRid)-$(Platform)" /> 
        
           </Target>

I think this is dead code. Nothing sets _IsBootstrapping.

Afaik, the patching of the runtime graph with non-portable rids is working well since .NET 7 when the runtime repo assumed that responsibility.

ViktorHofer · 2023-11-15T19:21:23Z

This confuses me:

    "centos.8-x64": {
      "#import": [
        "linux"
      ]
    }

Shouldn't this import linux-x64?

Here's the live built RID graph: PortableRuntimeIdentifierGraph.json

Yes, that's the issue. The current portable runtime json file has the following entry:

"centos.8-x64": {
      "#import": [
        "linux-x64"
      ]
    }

This needs a fix in runtime.

ViktorHofer · 2023-11-15T20:23:09Z

src/SourceBuild/content/repo-projects/runtime.proj

@@ -16,7 +16,7 @@
    <RuntimeOS>$(NETCoreSdkRuntimeIdentifier.Substring(0, $(_platformIndex)))</RuntimeOS>

    <_platformIndex>$(NETCoreSdkPortableRuntimeIdentifier.LastIndexOf('-'))</_platformIndex>
-    <BaseOS>$(NETCoreSdkPortableRuntimeIdentifier.Substring(0, $(_platformIndex)))</BaseOS>
+    <BaseOS>$(NETCoreSdkPortableRuntimeIdentifier.Substring(0, $(_platformIndex)))-$(Platform)</BaseOS>


@tmds I had to add the Platform property here so that AdditionalRuntimeIdentifierParent is set to the right value.

This needs to change as we now use a different "add current RID to RID graph" code in runtime (the one that originated in sdk): https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.NETCore.Platforms/src/UpdateRuntimeIdentifierGraph.cs

Please double check if this is correct. Should we change the name of this property to PortableRid (which is what sdk.proj uses)? Changing the property name would require a change in runtime.

I assume your pipelines pass BaseOS in as well?

This needs to change as we now use a different "add current RID to RID graph" code in runtime (the one that originated in sdk): https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.NETCore.Platforms/src/UpdateRuntimeIdentifierGraph.cs

Yes, it seems that the task that updated the old runtime graph didn't expect this to include an architecture (which is why this is named OS and not Rid), while the new task that updates the new graphs expects this to include an architecture.

Please double check if this is correct. Should we change the name of this property to PortableRid (which is what sdk.proj uses)? Changing the property name would require a change in runtime.

If it includes an architecture, it makes more sense to end with a Rid suffix.

Yes, this line needs to change: https://github.com/dotnet/runtime/blob/487d7f010b90a82178d55eea21e72dc6dedf9d5d/eng/SourceBuild.props#L45.

Consume portable RID graph from runtime

d1e0c88

1. Consume portable RID graph from runtime (subsequent PR will delete it from sdk). 2. Use the portable RID graph in source build projects 3. Delete the mutate RID graph and invocation as that's already handled directly in runtime.

ViktorHofer requested review from dsplaisted and elinor-fung November 10, 2023 18:40

ViktorHofer requested a review from a team as a code owner November 10, 2023 18:40

ViktorHofer commented Nov 10, 2023

View reviewed changes

ViktorHofer added a commit to dotnet/sdk that referenced this pull request Nov 10, 2023

Let runtime supply the portable RID graph

a91b080

Contributes to #35750 Related: dotnet/installer#17760

ViktorHofer mentioned this pull request Nov 10, 2023

Let runtime supply the portable RID graph dotnet/sdk#36807

Merged

MichaelSimons approved these changes Nov 13, 2023

View reviewed changes

ViktorHofer referenced this pull request Nov 13, 2023

Remove runtime-portable build.

9ce6a45

dsplaisted approved these changes Nov 13, 2023

View reviewed changes

elinor-fung approved these changes Nov 13, 2023

View reviewed changes

MichaelSimons mentioned this pull request Nov 14, 2023

AddRidToRuntimeJson doesn't work for BSD or musl based distros dotnet/source-build#303

Closed

tmds reviewed Nov 15, 2023

View reviewed changes

ViktorHofer added 3 commits November 15, 2023 20:24

Update GenerateBundledVersions.targets

d3f66d1

Update GenerateBundledVersions.targets

363514d

Update runtime.proj

8152f8a

ViktorHofer commented Nov 15, 2023

View reviewed changes

ViktorHofer merged commit 5037a63 into main Nov 16, 2023

ViktorHofer deleted the UseRIDGraphFromRuntime branch November 16, 2023 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consume portable RID graph from runtime #17760

Consume portable RID graph from runtime #17760

ViktorHofer commented Nov 10, 2023 •

edited

Loading

ViktorHofer Nov 10, 2023 •

edited

Loading

tmds Nov 15, 2023

MichaelSimons Nov 13, 2023

MichaelSimons commented Nov 14, 2023

ViktorHofer commented Nov 14, 2023

tmds Nov 15, 2023

ViktorHofer Nov 15, 2023 •

edited

Loading

tmds commented Nov 15, 2023 •

edited

Loading

ViktorHofer commented Nov 15, 2023

tmds commented Nov 15, 2023

ViktorHofer commented Nov 15, 2023

tmds commented Nov 15, 2023 •

edited

Loading

ViktorHofer commented Nov 15, 2023 •

edited

Loading

ViktorHofer Nov 15, 2023 •

edited

Loading

tmds Nov 16, 2023

Consume portable RID graph from runtime #17760

Consume portable RID graph from runtime #17760

Conversation

ViktorHofer commented Nov 10, 2023 • edited Loading

ViktorHofer Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

tmds Nov 15, 2023

Choose a reason for hiding this comment

MichaelSimons Nov 13, 2023

Choose a reason for hiding this comment

MichaelSimons commented Nov 14, 2023

ViktorHofer commented Nov 14, 2023

tmds Nov 15, 2023

Choose a reason for hiding this comment

ViktorHofer Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

tmds commented Nov 15, 2023 • edited Loading

ViktorHofer commented Nov 15, 2023

tmds commented Nov 15, 2023

ViktorHofer commented Nov 15, 2023

tmds commented Nov 15, 2023 • edited Loading

ViktorHofer commented Nov 15, 2023 • edited Loading

ViktorHofer Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

tmds Nov 16, 2023

Choose a reason for hiding this comment

ViktorHofer commented Nov 10, 2023 •

edited

Loading

ViktorHofer Nov 10, 2023 •

edited

Loading

ViktorHofer Nov 15, 2023 •

edited

Loading

tmds commented Nov 15, 2023 •

edited

Loading

tmds commented Nov 15, 2023 •

edited

Loading

ViktorHofer commented Nov 15, 2023 •

edited

Loading

ViktorHofer Nov 15, 2023 •

edited

Loading