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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d70086
Add to metadata, don't overwrite
jtschuster Jan 11, 2024
51f8e92
Don't use multiline Update string
jtschuster Jan 11, 2024
d3714c4
Use a little batching to only change the current framework reference
jtschuster Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@
for Microsoft.NETCore.App.Runtime.<rid> and Microsoft.NETCore.App.Crossgen2.<rid>.
-->
<ItemGroup Condition="'$(DotNetBuildFromSource)' == 'true'">
<KnownFrameworkReference Update="@(KnownFrameworkReference->WithMetadataValue('Identity', 'Microsoft.NETCore.App')->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))">
<RuntimePackRuntimeIdentifiers>$(PackageRID)</RuntimePackRuntimeIdentifiers>
</KnownFrameworkReference>
<KnownCrossgen2Pack Update="@(KnownCrossgen2Pack->WithMetadataValue('Identity', 'Microsoft.NETCore.App.Crossgen2')->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))">
<Crossgen2RuntimeIdentifiers>$(PackageRID)</Crossgen2RuntimeIdentifiers>
</KnownCrossgen2Pack>
<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

jtschuster marked this conversation as resolved.
Show resolved Hide resolved
<KnownCrossgen2Pack Update="@(KnownCrossgen2Pack
->WithMetadataValue('Identity', 'Microsoft.NETCore.App.Crossgen2')
->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))"
Crossgen2RuntimeIdentifiers="%(Crossgen2RuntimeIdentifiers);$(PackageRID)"/>
<!-- Avoid references to Microsoft.AspNetCore.App.Runtime.<rid> -->
<KnownFrameworkReference Remove="Microsoft.AspNetCore.App" />
</ItemGroup>
Expand Down
Loading