Don't use the portable RIDs for older runtimes if it's empty

[marcpopMSFT]

Fixes #52565

NuGet Package Explorer is an unusual application where even when you publish with a RID set, it doesn't pass that RID into ProcessFrameworkReferences. In that case we default to any. Prior to #52407, we had a check that if our list of supported target rids came back empty, we'd default to non-portable but that doesn't work for source build publishing.

Portable rids were added into GenerateBundledVersions.targets starting in net10 known crossgen packs so if you target an older runtime and don't pass a RID into PFR, we'll end up trying to use the empty portable rid list. This is the simplest fix of going back to non-portable if we don't have that list. Alternatively, we could add portable rids to GBV.targets for all/most downlevel pack definitions but I don't know the impact that'll have.

CC @tmds @agocke

@baronfel @dsplaisted we cannot take the backport of #52407 as is as it'll break scenarios like NPE. Do you know why the portable rid list and the non-portable RID list for 10+ are identical (trying to understand the point of separating them if they're the same)? Are they different in source built SDKs?

[Copilot]

Pull request overview

This PR adjusts RID selection for Crossgen2 and ILCompiler tool packs so that “portable” RIDs are not considered when their metadata is effectively empty, preventing incorrect fallback behavior on older runtimes.

Changes:

• Change parsing of RuntimeIdentifiers and PortableRuntimeIdentifiers metadata to use Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries) to avoid empty entries.
• Update the usePortable decision condition to require that packSupportedPortableRuntimeIdentifiers is non-empty before preferring portable RIDs.

[marcpopMSFT]

@jkoritzinsky I believe you added the portable rid handling in this PR but it's only for net10 and higher: https://github.com/dotnet/dotnet/pull/2140/files Was the intent of that change to only have the rid difference for net10+? Trying to understand if it's better to do the fallback in this PR or to add a portable rid list to downlevel pack definitions? Also, why are the portable and non-portable lists the same? Are they different in some situations that aren't captured in our generate bundled versions targets (source build for example?)?

[tmds]

Do you know why the portable rid list and the non-portable RID list for 10+ are identical (trying to understand the point of separating them if they're the same)? Are they different in source built SDKs?

The non-portable list = portable list (Microsoft provided rids) + non-portable rids (source-build provided rids).

When the SDK is asked to target one of the non-portable rids it will use that, otherwise it will use the portable rid:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

Lines 851 to 854 in 701f795

	// Prefer portable when the "supported RID" for the tool pack is the same RID as the "supported portable RID".
	// This makes non-portable SDKs behave the same as portable SDKs except for the specific cases added to "supported", such as targeting the non-portable RID.
	// This also ensures that targeting common RIDs doesn't require any non-portable assets that aren't packaged in the SDK by default.
	// Due to size concerns, the non-portable ILCompiler and Crossgen2 aren't included by default in non-portable SDK distributions.

Besides it including the RID of the SDK being built (in Microsoft.NETCoreSdk.BundledVersions.props), during the vmr build the rid of the build SDK is also added, like in https://github.com/dotnet/dotnet/blob/45d1ed746b88c992e7bd1a2ebe2b6e6bd531d3be/src/aspnetcore/eng/tools/GenerateFiles/Directory.Build.targets.in#L114.

[tmds]

@marcpopMSFT from a user perspective, we want the source-built SDK use portable artifacts just like the Microsoft SDK except when the user specifically opts into non-portable by setting RuntimeIdentifier to a non-portable rid.

[marcpopMSFT]

@marcpopMSFT from a user perspective, we want the source-built SDK use portable artifacts just like the Microsoft SDK except when the user specifically opts into non-portable by setting RuntimeIdentifier to a non-portable rid.

@tmds Is the intent that we should have portable rids passed through for the known packs for all downlevel targets then? The fix here ensures we don't use portable if there aren't any but it wasn't clear to me if the actual fix should be to add those to the GBV.targets. Do the source built SDKs add additional portable RIDs for net9 and below that are just ignored today or is there additional msbuild logic outside the SDK to add those?

[jkoritzinsky]

I think the correct fix here would be to fall back to treating the RuntimeIdentifiers list as the "portable" runtime identifiers (ie when packSupportedPortableRuntimeIdentifiers is empty, set it to packSupportedRuntimeIdentifiers and leave the rest of the logic as-is).

As source-build doesn't bundle/produce non-portable assets for downlevel TFMs (and we don't add the non-portable RID as a supported RID for that case anyway in GenerateBundledVersions.targets), that should provide us the correct experience.

[marcpopMSFT]

I think the correct fix here would be to fall back to treating the RuntimeIdentifiers list as the "portable" runtime identifiers (ie when packSupportedPortableRuntimeIdentifiers is empty, set it to packSupportedRuntimeIdentifiers and leave the rest of the logic as-is).

As source-build doesn't bundle/produce non-portable assets for downlevel TFMs (and we don't add the non-portable RID as a supported RID for that case anyway in GenerateBundledVersions.targets), that should provide us the correct experience.

Ahh, so instead of falling back to non-portable in the logic, just make the portable list match the non-portable list. That seems like it would work as well.

[marcpopMSFT]

Updated the fix per jkoritzinsky above. We'll fall back to using the non-portable list if the portable one is empty. This will need to be cherry-picked into the other backport PRs once merged.

[ViktorHofer]

This showed up in a re-bootstrap PR similar to how the vendor found it in #52565. Would be good to add a test in a follow-up to detect this earlier.

Needs to be ported into https://github.com/dotnet/sdk/tree/release/11.0.1xx-preview1 as well

[ViktorHofer]

/backport to release/11.0.1xx-preview1

[github-actions]

Started backporting to release/11.0.1xx-preview1 (link to workflow run)

[tmds]

For .NET 10, this goes together with #52485 and #52486.

[marcpopMSFT]

For .NET 10, this goes together with #52485 and #52486.
Picked this change into those PRs.

Rerunning tests here as the test legs timed out.

[marcpopMSFT]

/ba-g template tests are known issue. Test leg timeouts are helix outage. This is merged into the preview 1 branch already.