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

Simplify .NETFramework tfms by avoiding the "-windows" RID #58495

Closed
ViktorHofer opened this issue Sep 1, 2021 · 3 comments · Fixed by #58558
Closed

Simplify .NETFramework tfms by avoiding the "-windows" RID #58495

ViktorHofer opened this issue Sep 1, 2021 · 3 comments · Fixed by #58558

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented Sep 1, 2021

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). For any project that includes a RID specific .NETFramework or .NETStandard tfm, a NuGet.config file must be present next to the solution file so that Visual Studio doesn't attempt to restore these projects as the mentioned hack doesn't work inside Visual Studio. FWIW, generating such NuGet.config file and placing it next to solution files when required is currently handled by slngen.proj:

<Target Name="AddNuGetConfigToSolution"

I propose that we 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. 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.

cc @joperezr @ericstj

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner labels Sep 1, 2021
@ghost
Copy link

ghost commented Sep 1, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

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). For any project that includes a RID specific .NETFramework or .NETStandard tfm, a NuGet.config file must be present next to the solution file so that Visual Studio doesn't attempt to restore these projects as the mentioned hack doesn't work inside Visual Studio. FWIW, generating such NuGet.config file and placing it next to solution files when required is currently handled by slngen.proj:

<Target Name="AddNuGetConfigToSolution"
.

I propose that we 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. 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.

cc @joperezr @ericstj

Author: ViktorHofer
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@ericstj
Copy link
Member

ericstj commented Sep 1, 2021

I propose that we 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

Agreed.

Would we have some place that would catch if we got things wrong? I imagine "getting it wrong" would look like exposing a type def where we needed a forward.

I think if we told the SDK to raise runtime targets for .NETFramework then package testing would likely catch this. Not sure if PackageValidation would catch it, though that could be something to look into as well.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Sep 2, 2021

Would we have some place that would catch if we got things wrong? I imagine "getting it wrong" would look like exposing a type def where we needed a forward.

EDIT: Protection is a good idea to prevent this from regressing in the future.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 2, 2021
@ViktorHofer ViktorHofer added this to the 7.0.0 milestone Sep 3, 2021
@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Sep 3, 2021
ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue Sep 8, 2021
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 added a commit that referenced this issue Sep 10, 2021
* Simplfy .NETFramework tfms

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 #58495

* Warn on .NETFramework duplicate runtime assets

* Ignore .NEtFramework non Windows and non empty RIDs

Also cleaning up the packaging.targets file and removing
the ExcludeFromPackage option which isn't needed anymore as
target frameworks aren't excluded from packages produced in
dotnet/runtime anymore.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Oct 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants