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

Add NetCoreAppCurrent rid agnostic tfm to libs #64518

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jan 30, 2022

This change adds a NetCoreAppCurrent rid-less configuration which throws
a PNSE to the following libraries:

  • System.Console
  • System.IO.Compression.ZipFile
  • System.IO.Compression
  • System.IO.FileSystem.DriveInfo
  • System.IO.MemoryMappedFiles
  • System.Net.Http
  • System.Net.Mail
  • System.Net.NameResolution
  • System.Net.Primitives
  • System.Net.Requests
  • System.Net.WebSockets

System.Private.Runtime.InteropServices.JavaScript is excluded from that
list as it doesn't have a contract and hence can't generate a PNSE
assembly. Also it's currently a private assembly.

These are the only libraries that were missing a RID agnostic tfm.

Adding rid-less configurations, even if they just throw PNSEs helps with:

  • Platform enablement, i.e. a new "base" platforms like "Browser" usually
    require libraries that are RID specific to support them. By offering a
    RID agnostic PNSE tfm, projects can gradually support new platforms.
  • Making it possible to have ProjectReferences between libraries, i.e.
    imagine an microsoft.build.notargets / microsoft.build.traversal
    project with a $(NetCoreAppCurrent)-$(TargetOS) tfm which P2Ps all
    source projects. NuGet can't know that a $(NetCoreAppCurrent)-Linux
    tfm can reference a project with a $(NetCoreAppCurrent)-Unix tfm and
    will throw during restore, more precisely during the compatibility
    check. Ideally NuGet or the SDK would support targeting platforms from
    the runtime.json RID graph, including its compatiblity matrix but
    that isn't the case today.

Even though the added tfms add to the build, I have other changes
in the pipeline which depend on this one which will make the build
noticeably faster.

@ghost
Copy link

ghost commented Jan 30, 2022

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

Issue Details

This change adds a NetCoreAppCurrent rid-less configuration which throws
a PNSE to the following libraries:

  • System.Console
  • System.IO.Compression.ZipFile
  • System.IO.Compression
  • System.IO.FileSystem.DriveInfo
  • System.IO.MemoryMappedFiles
  • System.Net.Http
  • System.Net.Mail
  • System.Net.NameResolution
  • System.Net.Primitives
  • System.Net.Requests
  • System.Net.WebSockets
  • System.Private.Runtime.InteropServices.JavaScript

These are the only libraries that were missing a RID agnostic tfm.

Adding rid-less configurations, even if they just throw PNSEs helps with:

  • Platform enablement, i.e. a new "base" platforms like "Browser" usually
    require libraries that are RID specific to support them. By offering a
    RID agnostic PNSE tfm, projects can gradually support new platforms.
  • Making it possible to have ProjectReferences between libraries, i.e.
    imagine an microsoft.build.notargets / microsoft.build.traversal
    project with a $(NetCoreAppCurrent)-$(TargetOS) tfm which P2Ps all
    source projects. NuGet can't know that a $(NetCoreAppCurrent)-Linux
    tfm can reference a project with a $(NetCoreAppCurrent)-Unix tfm and
    will throw during restore, more precisely during the compatibility
    check. Ideally NuGet or the SDK would support targeting platforms from
    the runtime.json RID graph, including its compatiblity matrix but
    that isn't the case today.

Even though the added tfms add to the build, I have other changes
in the pipeline which depend on this one which will make the build
noticeably faster.

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: -

This change adds a NetCoreAppCurrent rid-less configuration which throws
a PNSE to the following libraries:
- System.Console
- System.IO.Compression.ZipFile
- System.IO.Compression
- System.IO.FileSystem.DriveInfo
- System.IO.MemoryMappedFiles
- System.Net.Http
- System.Net.Mail
- System.Net.NameResolution
- System.Net.Primitives
- System.Net.Requests
- System.Net.WebSockets

System.Private.Runtime.InteropServices.JavaScript is excluded from that
list as it doesn't have a contract and hence can't generate a PNSE
assembly. Also it's currently a private assembly.

These are the only libraries that were missing a RID agnostic tfm.

Adding rid-less configurations, even if they just throw PNSEs helps with:
- Platform enablement, i.e. a new "base" platforms like "Browser" usually
  require libraries that are RID specific to support them. By offering a
  RID agnostic PNSE tfm, projects can gradually support new platforms.
- Making it possible to have ProjectReferences between libraries, i.e.
  imagine an microsoft.build.notargets / microsoft.build.traversal
  project with a `$(NetCoreAppCurrent)-$(TargetOS)` tfm which P2Ps all
  source projects. NuGet can't know that a $(NetCoreAppCurrent)-Linux
  tfm can reference a project with a $(NetCoreAppCurrent)-Unix tfm and
  will throw during restore, more precisely during the compatibility
  check. Ideally NuGet or the SDK would support targeting platforms from
  the runtime.json RID graph, including its compatiblity matrix but
  that isn't the case today.

Even though the added tfms add to the build, I have other changes
in the pipeline which depend on this one which will make the build
noticeably faster.
@ViktorHofer
Copy link
Member Author

I looked through the other eight libraries and they don't seem to have a suitable platform agnostic configuration already. Please let me know if you have any other feedback and if not, let's get this in :)

@ericstj
Copy link
Member

ericstj commented Jan 31, 2022

FYI @dotnet/area-system-io @dotnet/ncl @dotnet/area-system-console

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -31,21 +31,29 @@
<PropertyGroup>
<ILLinkDirectory>$(MSBuildThisFileDirectory)ILLink\</ILLinkDirectory>
</PropertyGroup>
<!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
<PropertyGroup>
<TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
Copy link
Member

@safern safern Feb 1, 2022

Choose a reason for hiding this comment

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

Can we or is it worth to this globally for all projects that multitarget or is that an expensive call? I ask cause I see this property being set in multiple projects.

I guess it won't work because of DesignTimeBuild but I figured I'd ask.

Copy link
Member Author

Choose a reason for hiding this comment

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

The SDK already does this but in the TargetFrameworkInference.targets file which means the property won't be for properties inside the project file. Because of that, we are setting it ourselves in exactly the same way the SDK sets it and the result will be cached and the duplicate is avoided.

We can't set this globally as it relies on the TargetFramework prop which isn't available at that point.

@ViktorHofer ViktorHofer merged commit daf77fc into dotnet:main Feb 1, 2022
@ViktorHofer ViktorHofer deleted the NetCoreAppLibAgnosticTfm branch February 1, 2022 06:49
@ghost ghost locked as resolved and limited conversation to collaborators Mar 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants