Simplify .NETFramework tfms #58558
Simplify .NETFramework tfms #58558
Conversation
|
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsLibraries which target .NET Framework usually have rid agnostic tfms, There is yet another reason that requires .NETFramework tfms to be RID NuGet doesn't support setting a TargetPlatform in the TargetFramework As NuGet will likely never support RID specific .NETFramework tfm Remove all "TargetFrameworkSuffixes" / TargetPlatforms / RIDs Explictly don't set TargetsWindows to true for .NETFramework builds as Also cleaning up the packaging.targets file to make this change easier. Fixes #58495
|
ViktorHofer
force-pushed
the
SimplfyNetFxConfigurations
branch
from
307af17 to
8f014be
Compare
There was a problem hiding this comment.
Were you able to test if the package testing leg will catch if we somehow get this wrong? If it doesn't catch it then I think we should be able to make a small change to raise runtime-targets for .NETFramework. I see that NuGet does create them.
"compile": {
"ref/net461/Microsoft.Win32.Registry.dll": {}
},
"runtime": {
"lib/net461/Microsoft.Win32.Registry.dll": {}
},
"runtimeTargets": {
"runtimes/win/lib/net461/Microsoft.Win32.Registry.dll": {
"assetType": "runtime",
"rid": "win"
}
}
src/libraries/System.Threading.AccessControl/src/System.Threading.AccessControl.csproj
Outdated
Show resolved
Hide resolved
Do you have an example of what to test for? |
|
Try to force a case where a package would have |
Tried that with ProtectedData and neither PackageValidation nor the runtime-repo specific testPackages infrastructure catches that.
|
|
I think we can catch it if we turn on |
|
Looks like setting |
|
That worked quite well 👍 Thanks Eric. At some point I need to familiarize myself with the project.assets.json format... |
Libraries which target .NET Framework usually have rid agnostic tfms, i.e. net461. If the library targets netstandard2.0-windows as well, the .NET Framework tfm must be rid specific, as rid specific .NET Framework apps would otherwise pick the .NETStandard asset over the .NETFramework one (based on the RID compatibility rules) There is yet another reason that requires .NETFramework tfms to be RID specific, which is when a project P2Ps other projects which are rid-specific. Without the RID specific .NETFramework tfm, a compatible .NETStandard asset would be picked instead. NuGet doesn't support setting a TargetPlatform in the TargetFramework alias when targeting .NETFramework or .NETStandard. Any such tfms in dotnet/runtime are currently leveraging a hack that strips the TargetPlatform / TargetFrameworkSuffix away during restore and packaging (as NuGet Pack uses the project.assets.json file). As NuGet will likely never support RID specific .NETFramework tfm aliases, the distinction between a RID specific and a RID agnostic .NETFramework tfm is unnecessary. Remove all "TargetFrameworkSuffixes" / TargetPlatforms / RIDs (whatever you would like to call them) from .NETFramework tfms and let the packaging targets handle the cases where a RID specific asset is required in the package. Explictly don't set TargetsWindows to true for .NETFramework builds as the Targets* properties are infered from the platform / suffix and many projects make the assumption that net461 (without the "-windows" part) doesn't set TargetsWindows=true. Fixes dotnet#58495
ViktorHofer
force-pushed
the
SimplfyNetFxConfigurations
branch
from
8f014be to
54e0de2
Compare
With dotnet/runtime#58558, .NETFramework tfms which include a RID (i.e. net461-windows) aren't used and supported in dotnet/runtime anymore. Removing the support from the TargetFramework.Sdk as .NETFramework tfms with a RID aren't supported by NuGet anyways and throw.
@ericstj @Anipik any idea why the Unix asset is considered compatible for the .NETFramework runtime targets? |
This is what I said you'd want to look out for above:
The toolchain has no knowledge of what RID's are "valid" for a particular framework, it just provides runtimeTargets in case they might be used. On .NETCore it's the host's decision to decide if it happens to use one of those (based on the RID fallback graph it has for the particular runtime it's running on). Package testing is trying to make sure that any asset that "might" apply for any RID meets our test criteria. Since it's not a scenario to restore .NETFramework for non-Windows RIDs I don't think we need to test them (nor force our packages to add non-windows-RID-specific netframework assets). |
| <!-- Don't verify RID specific libs that aren't compatible on .NETFramework, i.e. Unix. --> | ||
| <RuntimeLibToTest Remove="@(RuntimeLibToTest)" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and | ||
| '%(RuntimeLibToTest.RuntimeIdentifier)' != '' and | ||
| '%(RuntimeLibToTest.RuntimeIdentifier)' != 'win'" /> |
There was a problem hiding this comment.
I think you want startswith('win-') here. Here's where NuGet calculates the RID for classic CSProj:
https://github.com/dotnet/NuGet.BuildTasks/blob/b2f0c162994237c4a9c23af79d95f79847499ebd/src/Microsoft.NuGet.Build.Tasks/Microsoft.NuGet.targets#L89-L113
There was a problem hiding this comment.
win- wouldn't catch "win" itself so I guess checking (eq "win" or startswith("win-")) is necessary:
|
Hven't fully looked at the PR yet (going over it now) but looks like most of the discussion about regression testing above was in order to catch cases where our logic to automatically add the rid-specific asset failed. That said, do we also have some sort of coverage for cases where P2Ps of a net461 project would pick the netstandard2.0-windows over the net461 ridless asset? Or is it that this will now never be an issue because we won't have any net461-win configurations anymore so all P2Ps will always prefer other net461 configs over netstandard2.0-win configs? |
Correct. With .NETFramework RID agnostic project configurations only, a compatible (RID specific) |
|
And do we have regression testing to ensure that nobody will add a netfx-win config by mistake in the future? As that would bring the P2P problem back. |
|
|
Final general comment before actually looking into the PR 😄: Seems like in the PR description you are not making any special note about removing the support for |
| @@ -21,17 +21,15 @@ System.Security.AccessControl.SemaphoreSecurity</PackageDescription> | |||
| <IsPartialFacadeAssembly Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework'">true</IsPartialFacadeAssembly> | |||
| <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(IsPartialFacadeAssembly)' != 'true' and '$(TargetsWindows)' != 'true'">SR.PlatformNotSupported_AccessControl</GeneratePlatformNotSupportedAssemblyMessage> | |||
| </PropertyGroup> | |||
| <ItemGroup Condition="'$(TargetsWindows)' == 'true'"> | |||
There was a problem hiding this comment.
This is technically a change as we were compiling this file before on the netfx asset. That said it was obviously a mistake so it was a good catch to find here
There was a problem hiding this comment.
Changes look good to me, thanks for fixing this @ViktorHofer I really like the simplified configurations much better. Might be good just to super double check that your changes didn't actually modified the resulting packages by diffing them before and after your change.
With dotnet/runtime#58558, .NETFramework tfms which include a RID (i.e. net461-windows) aren't used and supported in dotnet/runtime anymore. Removing the support from the TargetFramework.Sdk as .NETFramework tfms with a RID aren't supported by NuGet anyways and throw.
* Don't strip .NETFramework tfms With dotnet/runtime#58558, .NETFramework tfms which include a RID (i.e. net461-windows) aren't used and supported in dotnet/runtime anymore. Removing the support from the TargetFramework.Sdk as .NETFramework tfms with a RID aren't supported by NuGet anyways and throw. * Update Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets
ghost
locked as resolved and limited conversation to collaborators
Libraries which target .NET Framework usually have rid agnostic tfms,
i.e. net461. If the library targets netstandard2.0-windows as well,
the .NET Framework tfm must be rid specific, as rid specific
.NET Framework apps would otherwise pick the .NETStandard asset over
the .NETFramework one (based on the RID compatibility rules)
There is yet another reason that requires .NETFramework tfms to be RID
specific, which is when a project P2Ps other projects which are
rid-specific. Without the RID specific .NETFramework tfm, a compatible
.NETStandard asset would be picked instead.
NuGet doesn't support setting a TargetPlatform in the TargetFramework
alias when targeting .NETFramework or .NETStandard. Any such tfms in
dotnet/runtime are currently leveraging a hack that strips the
TargetPlatform / TargetFrameworkSuffix away during restore and packaging
(as NuGet Pack uses the project.assets.json file).
As NuGet will likely never support RID specific .NETFramework tfm
aliases, the distinction between a RID specific and a RID agnostic
.NETFramework tfm is unnecessary.
Remove all "TargetFrameworkSuffixes" / TargetPlatforms / RIDs
(whatever you would like to call them) from .NETFramework tfms and let
the packaging targets handle the cases where a RID specific asset is
required in the package.
Explictly don't set TargetsWindows to true for .NETFramework builds as
the Targets* properties are infered from the platform / suffix and
many projects make the assumption that net461
(without the "-windows" part) does mean TargetsWindows != true.
Also cleaning up the packaging.targets file and removing
the
ExcludeFromPackageoption which isn't needed anymore astarget frameworks aren't excluded from packages produced in
dotnet/runtime anymore.
Fixes #58495