Skip to content
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.

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

Fix PortableRuntimeIdentifierGraph.json not found during runtime repo source build #90695

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

elinor-fung
Copy link
Member

Fixes dotnet/source-build#3592
Will need to go to rc1 as well

Since the full RID graph isn't supposed to be updated any more, stop setting the BundledRuntimeIdentifierGraphFile property, which is currently breaking source-build bootstrapping scenarios.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 16, 2023
@ghost ghost assigned elinor-fung Aug 16, 2023
@lewing lewing closed this Aug 16, 2023
@lewing lewing reopened this Aug 16, 2023
@elinor-fung elinor-fung merged commit 471d1a1 into main Aug 17, 2023
166 of 172 checks passed
@elinor-fung elinor-fung deleted the elinor-fung-patch-1 branch August 17, 2023 04:26
@elinor-fung
Copy link
Member Author

/backport to release/8.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5886760639

<!-- Keep in sync with outputs defined in Microsoft.NETCore.Platforms.csproj. -->
<BundledRuntimeIdentifierGraphFile>$([MSBuild]::NormalizePath('$(ArtifactsBinDir)', 'Microsoft.NETCore.Platforms', 'runtime.json'))</BundledRuntimeIdentifierGraphFile>
<BundledRuntimeIdentifierGraphFile Condition="!Exists('$(BundledRuntimeIdentifierGraphFile)')">$([MSBuild]::NormalizePath('$(LibrariesProjectRoot)', 'Microsoft.NETCore.Platforms', 'src', 'runtime.json'))</BundledRuntimeIdentifierGraphFile>
</PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

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

@elinor-fung @ViktorHofer

We've added these lines so the build SDK would know how the rid being built fits in the graph.
I'd be surprised if we can just remove them, as that would mean they were not needed.

Copy link
Member

@ViktorHofer ViktorHofer Aug 17, 2023

Choose a reason for hiding this comment

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

Doesn't the SDK now add the distro specific RID into the graph? cc @dsplaisted

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the SDK that gets built has the non-portable rid added to it.

That's not what these lines are for: they are for making the SDK that is during the build (for example: a portable Microsoft SDK) know the non-portable rid.

trungnt2910 added a commit to trungnt2910/dotnet-builds that referenced this pull request Aug 21, 2023
@tmds
Copy link
Member

tmds commented Sep 4, 2023

A consequence of this is the Microsoft.NETCore.App.deps.json no longer include a runtimes section on the stage 1 build because the non-portable rid is not known by the Microsoft SDK.

The rid is then known by the output SDK, so the next build (for the same rid, using that source-built SDK) does include a runtimes section.

Afaik, this runtimes section is only used when an application explicitly opts in to the graph. So even for the stage 1 SDK the impact is limited.

cc @elinor-fung @MichaelSimons @omajid

@ghost ghost locked as resolved and limited conversation to collaborators Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PortableRuntimeIdentifierGraph.json not found during runtime repo build
5 participants