-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 configuration to three ref projects #54146
Conversation
System.ComponentModel.Composition, System.Diagnostics.PerformanceCounter and System.Runtime.Caching. Helps with dotnet#54012
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsSystem.ComponentModel.Composition, System.Diagnostics.PerformanceCounter and System.Runtime.Caching. Helps with #54012
|
src/libraries/System.ComponentModel.Composition/ref/System.ComponentModel.Composition.csproj
Outdated
Show resolved
Hide resolved
...aries/System.Diagnostics.PerformanceCounter/ref/System.Diagnostics.PerformanceCounter.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Caching/ref/System.Runtime.Caching.csproj
Outdated
Show resolved
Hide resolved
...aries/System.Diagnostics.PerformanceCounter/ref/System.Diagnostics.PerformanceCounter.csproj
Show resolved
Hide resolved
|
||
namespace System.Runtime.Caching.Configuration | ||
{ | ||
#if NET5_0_OR_GREATER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually don't ifdef and just set this property (in this case in the src csproj) to include the platform attributes on downstream tfms: <IncludePlatformAttributes>true</IncludePlatformAttributes>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already set that propery, but it's not enough here because browser
is not recognized as a valid platform on these downstream tfms and the build would fail with:
error CA1418: The platform 'browser' is not a known platform name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@buyaa-n is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is, maybe we should disable the CA1418 for TFMs below 5.0
#if NET5_0_OR_GREATER | ||
if (OperatingSystem.IsBrowser()) | ||
{ | ||
throw new PlatformNotSupportedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this method should have annotated with [UnsupportedOSPlatform("browser")]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because you could implement your own custom version of IFileChangeNotificationSystem
that works on browser and pass it in via ObjectCache.Host
. We should only throw when we're using the default implementation.
System.ComponentModel.Composition, System.Diagnostics.PerformanceCounter and System.Runtime.Caching.
Helps with #54012