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.

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

Add non-portable RID to list of known runtime packs instead of overwriting #96858

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Add non-portable RID to list of known runtime packs instead of overwriting #96858

merged 3 commits into from
Jan 12, 2024

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Jan 11, 2024 •
edited
Loading

In #75597, for source-build we replace all the RIDs with only the PackageRID in the KnownFrameworkReference for NetCurrent when we should have only added an additional RID.

This was causing issues in crossbuilds of vertical builds. The crossgen and ilc projects that run within the build require a runtime for the build machine's RID, but it wasn't found in KnownFrameworkReference and the build failed during restore.

Part of reenabling Crossgen and ILC in arm64 crossbuild verticals dotnet/source-build#3698

@ghost ghost assigned jtschuster Jan 11, 2024
@ghost
Copy link

ghost commented Jan 11, 2024

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

Issue Details

In #75597, we replace all the RIDs with only the PackageRID in the KnownFrameworkReference for NetCurrent when we should have only added an additional RID.

This was causing issues in crossbuilds of vertical builds. The crossgen and ilc projects that run within the build require a runtime for the build machine's RID, but it wasn't found in KnownFrameworkReference and the build failed during restore.

Part of reenabling Crossgen and ILC in arm64 crossbuild verticals dotnet/source-build#3698

Author: jtschuster
Assignees: -
Labels:

area-Infrastructure-libraries
Milestone: -

ViktorHofer
ViktorHofer reviewed Jan 11, 2024
View reviewed changes
Directory.Build.targets Outdated
Comment on lines 25 to 28
<KnownFrameworkReference Update="@(KnownFrameworkReference
->WithMetadataValue('Identity', 'Microsoft.NETCore.App')
->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))"
RuntimePackRuntimeIdentifiers="%(RuntimePackRuntimeIdentifiers);$(PackageRID)" />
Copy link
Member

Choose a reason for hiding this comment

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

Does that Update statement with multiple lines actually work? I never saw this before.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like it might be working, but I just double checked and it looks like the filtering might not be working correctly even on a single line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, it needs to be a condition, not WithMetadataValue

Copy link
Member

@ViktorHofer ViktorHofer Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't use and item batching condition here. Such are less performant. Prefer msbuild item functions when possible.

Also, this currently works before with the item functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

In my repro project the item functions don't seem to work and will add the RID to each KnownFrameworkReference regardless of TFM or Identity. I'll see if there's a different way to do it with msbuild items functions.

<Project>
  <PropertyGroup>
    <NetCoreAppCurrent>net8.0</NetCoreAppCurrent>
    <PackageRid>linux-arm64</PackageRid>
  </PropertyGroup>

  <ItemGroup>
    <KnownFrameworkReference Include="Microsoft.NETCore.App"
      TargetFramework="net8.0"
      RuntimePackRuntimeIdentifiers="OriginalRids"
      />

    <KnownFrameworkReference Include="Microsoft.NETCore.App.NativeAOT"
      TargetFramework="net8.0"
      RuntimePackRuntimeIdentifiers="OriginalRids"
      />

    <KnownFrameworkReference Include="Microsoft.NETCore.App"
      TargetFramework="net7.0"
      RuntimePackRuntimeIdentifiers="OriginalRids2"
      />

    <KnownFrameworkReference Include="Microsoft.NETCore.App.NativeAOT"
      TargetFramework="net7.0"
      RuntimePackRuntimeIdentifiers="OriginalRids2"
      />


    <KnownCrossgen2Pack Include='Microsoft.NETCore.App.Crossgen2'
      Crossgen2RuntimeIdentifiers="OriginalRids"
      TargetFramework="net8.0" />

    <KnownCrossgen2Pack Include='Microsoft.NETCore.App.Crossgen3'
      Crossgen2RuntimeIdentifiers="OriginalRids"
      TargetFramework="net8.0" />

    <KnownCrossgen2Pack Include='Microsoft.NETCore.App.Crossgen2'
      Crossgen2RuntimeIdentifiers="OriginalRids2"
      TargetFramework="net7.0" />

    <KnownCrossgen2Pack Include='Microsoft.NETCore.App.Crossgen3'
      Crossgen2RuntimeIdentifiers="OriginalRids2"
      TargetFramework="net7.0" />

  </ItemGroup>

  <Target Name="MyModifyingTarget">
    <ItemGroup Condition="'$(NoBatch)' != 'true'">
      <KnownFrameworkReference
        Update="@(KnownFrameworkReference)"
        Condition="'%(Identity)' == 'Microsoft.NETCore.App' AND '%(TargetFramework)' == '$(NetCoreAppCurrent)'"
        RuntimePackRuntimeIdentifiers="%(RuntimePackRuntimeIdentifiers);$(PackageRID)" />

      <KnownCrossgen2Pack
        Update="@(KnownCrossgen2Pack)"
        Condition="'%(Identity)' == 'Microsoft.NETCore.App.Crossgen2' AND '%(TargetFramework)' == '$(NetCoreAppCurrent)'"
        Crossgen2RuntimeIdentifiers="%(Crossgen2RuntimeIdentifiers);$(PackageRID)"/>
    </ItemGroup>

    <ItemGroup Condition="'$(NoBatch)' == 'true'">
      <KnownFrameworkReference
        Update="@(KnownFrameworkReference)->WithMetadataValue('Identity', 'Microsoft.NETCore.App')->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)')"
        RuntimePackRuntimeIdentifiers="%(RuntimePackRuntimeIdentifiers);$(PackageRID)" />

      <KnownCrossgen2Pack
        Update="@(KnownCrossgen2Pack->WithMetadataValue('Identity', 'Microsoft.NETCore.App.Crossgen2')->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))"
        Crossgen2RuntimeIdentifiers="%(Crossgen2RuntimeIdentifiers);$(PackageRID)"/>
    </ItemGroup>

    <Message Importance="High" Text="KnownFrameworkReference:%0a%09Name: %(KnownFrameworkReference.Identity)%0a%09TFM: %(KnownFrameworkReference.TargetFramework)%0a%09RIDs: %(KnownFrameworkReference.RuntimePackRuntimeIdentifiers)" />
    <Message Importance="High" Text="KnownCrossgen2Pack:%0a%09Name: %(KnownCrossgen2Pack.Identity)%0a%09TFM: %(KnownCrossgen2Pack.TargetFramework)%0a%09RIDs: %(KnownCrossgen2Pack.Crossgen2RuntimeIdentifiers)" />
  </Target>
</Project>

With NoBatch=true

  KnownFrameworkReference:
        Name: Microsoft.NETCore.App
        TFM: net8.0
        RIDs: OriginalRids;linux-arm64
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App.NativeAOT
        TFM: net8.0
        RIDs: OriginalRids;linux-arm64
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App
        TFM: net7.0
        RIDs: OriginalRids2;linux-arm64
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App.NativeAOT
        TFM: net7.0
        RIDs: OriginalRids2;linux-arm64
  KnownCrossgen2Pack:
        Name: Microsoft.NETCore.App.Crossgen2
        TFM: net8.0
        RIDs: OriginalRids;linux-arm64
  KnownCrossgen2Pack:
        Name: Microsoft.NETCore.App.Crossgen3
        TFM: net8.0
        RIDs: OriginalRids;linux-arm64
  KnownCrossgen2Pack:
        Name: Microsoft.NETCore.App.Crossgen2
        TFM: net7.0
        RIDs: OriginalRids2;linux-arm64
  KnownCrossgen2Pack:
        Name: Microsoft.NETCore.App.Crossgen3
        TFM: net7.0
        RIDs: OriginalRids2;linux-arm64

With NoBatch=false

  KnownFrameworkReference:
        Name: Microsoft.NETCore.App
        TFM: net8.0
        RIDs: OriginalRids;linux-arm64
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App.NativeAOT
        TFM: net8.0
        RIDs: OriginalRids
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App
        TFM: net7.0
        RIDs: OriginalRids2
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App.NativeAOT
        TFM: net7.0
        RIDs: OriginalRids2
  KnownCrossgen2Pack:
        Name: Microsoft.NETCore.App.Crossgen2
        TFM: net8.0
        RIDs: OriginalRids;linux-arm64
  KnownCrossgen2Pack:
        Name: Microsoft.NETCore.App.Crossgen3
        TFM: net8.0
        RIDs: OriginalRids
  KnownCrossgen2Pack:
        Name: Microsoft.NETCore.App.Crossgen2
        TFM: net7.0
        RIDs: OriginalRids2
  KnownCrossgen2Pack:
        Name: Microsoft.NETCore.App.Crossgen3
        TFM: net7.0
        RIDs: OriginalRids2

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that half fixed it, but it still looks like it matches on the name of each item only, so it will update all 'Microsoft.NETCore.App' items in the group, regardless of the TFM, so net7.0 item will also have the new RID.

  KnownFrameworkReference:
        Name: Microsoft.NETCore.App
        TFM: net8.0
        RIDs: OriginalRids;linux-arm64
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App.NativeAOT
        TFM: net8.0
        RIDs: OriginalRids
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App
        TFM: net7.0
        RIDs: OriginalRids;linux-arm64
  KnownFrameworkReference:
        Name: Microsoft.NETCore.App.NativeAOT
        TFM: net7.0
        RIDs: OriginalRids

We could batch only the items with the Identity 'Microsoft.NETCore.App' on TargetFramework == NetCurrent to reduce the overhead, or assume it won't be an issue if we haven't already hit an issue where a previous TFM has the additional RID.

Copy link
Member

Choose a reason for hiding this comment

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

I think you are doing something wrong. <KnownFrameworkReference Update="@(KnownFrameworkReference->WithMetadataValue('Identity', 'Microsoft.NETCore.App')->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)')) ... works for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I might be doing something wrong in the repro, I'll try running a source build and check the binlog to test the change.

Copy link
Member Author

@jtschuster jtschuster Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the source build all the items in the KnownFrameworkReferences itemgroup with the name Microsoft.NETCore.App are adding the PackageRid to their metadata, no matter the TargetFramework metadata. In a few places we do something like this:

<KnownFrameworkReference Update="Microsoft.NETCore.App">
        <RuntimePackRuntimeIdentifiers 
             Condition="'%(TargetFramework)' == '$(NetCoreAppCurrent)'">%(RuntimePackRuntimeIdentifiers);$(PackageRID)</RuntimePackRuntimeIdentifiers>
</KnownFrameworkReference>

<KnownCrossgen2Pack Update="Microsoft.NETCore.App.Crossgen2">
        <Crossgen2RuntimeIdentifiers
            Condition="'%(TargetFramework)' == '$(NetCoreAppCurrent)'">%(Crossgen2RuntimeIdentifiers);$(PackageRID)</Crossgen2RuntimeIdentifiers>
</KnownCrossgen2Pack>

It uses some batching, but I can't find another way to only update the item with the latest TFM.

Copy link
Member

Choose a reason for hiding this comment

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

OK I was able to reproduce this. This seems to be an msbuild issue. Filed dotnet/msbuild#9636

@am11 am11 mentioned this pull request Jan 11, 2024
Open
This was referenced Jan 11, 2024
Closed
Open
Closed
Closed
@build-analysis build-analysis bot mentioned this pull request Jan 12, 2024
Closed
@jtschuster jtschuster mentioned this pull request Jan 12, 2024
Closed
@jtschuster jtschuster merged commit 5e24539 into dotnet:main Jan 12, 2024
174 of 180 checks passed
@jtschuster jtschuster mentioned this pull request Jan 18, 2024
Merged
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@jtschuster @tmds
Add non-portable RID to list of known runtime packs instead of overwr…
b0d547e 
…iting in source-build (dotnet#96858)

In dotnet#75597, for source-build we replace all the RIDs with only the `PackageRID` in the `KnownFrameworkReference` for `NetCurrent` when we should have only added an additional RID.

This was causing issues in crossbuilds of vertical builds. The crossgen and ilc projects that run within the build require a runtime for the build machine's RID, but it wasn't found in `KnownFrameworkReference` and the build failed during restore.

Part of reenabling Crossgen and ILC in arm64 crossbuild verticals dotnet/source-build#3698
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@steveisok steveisok Awaiting requested review from steveisok

Assignees

@jtschuster jtschuster

Labels
area-Infrastructure-libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jtschuster @ViktorHofer