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

Avoid S.R.I.RuntimeInformation package dependency #85642

Merged

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented May 2, 2023

Contributes to #85641

System.Runtime.InteropServices.RuntimeInformation/4.3.0 is being referenced in a few .NET Framework builds. The reference to that package is undesirable as it brings in the entire netstandard1.x dependency graph.

Instead, use System.Environment.OSVersion which returns "Microsoft
Windows NT" on .NET Framework, Mono and .NETCoreApp runtimes.

cc @dotnet/area-dependencymodel @dotnet/area-extensions-hosting @dotnet/area-extensions-logging

@ViktorHofer ViktorHofer requested review from ericstj and a team May 2, 2023 11:02
@ViktorHofer ViktorHofer self-assigned this May 2, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 2, 2023
@ViktorHofer ViktorHofer marked this pull request as draft May 2, 2023 19:47
@ViktorHofer ViktorHofer force-pushed the AvoidRuntimeInformationPkgDependency branch from 3a524df to 4238e86 Compare May 2, 2023 19:58
@ViktorHofer ViktorHofer marked this pull request as ready for review May 2, 2023 20:00
@ViktorHofer ViktorHofer added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 2, 2023
@ghost
Copy link

ghost commented May 2, 2023

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

Issue Details

Contributes to #85641

System.Runtime.InteropServices.RuntimeInformation/4.3.0 is being referenced in a few .NET Framework builds. The reference to that package is undesirable as it brings in the entire netstandard1.x dependency graph.

Instead, use System.Environment.OSVersion which returns "Microsoft
Windows NT" on .NET Framework, Mono and .NETCoreApp runtimes.

cc @dotnet/area-dependencymodel @dotnet/area-extensions-hosting @dotnet/area-extensions-logging

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries, needs-area-label

Milestone: -

@ViktorHofer ViktorHofer force-pushed the AvoidRuntimeInformationPkgDependency branch 3 times, most recently from a313a67 to 747bd18 Compare May 2, 2023 20:16
Contributes to dotnet#85641

System.Runtime.InteropServices.RuntimeInformation/4.3.0 is being
referenced in a few .NET Framework builds. The reference to that package
is undesirable as it brings in the entire netstandard1.x dependency
graph.

Instead, use System.Environment.OSVersion which returns "Microsoft
Windows NT" on .NET Framework, Mono and .NETCoreApp runtimes.
@ViktorHofer ViktorHofer force-pushed the AvoidRuntimeInformationPkgDependency branch from 747bd18 to 1754a36 Compare May 2, 2023 20:22
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, @tarekgh @buyaa-n have a look please.

@@ -106,11 +106,15 @@ public static IServiceCollection AddWindowsService(this IServiceCollection servi

private static void AddWindowsServiceLifetime(IServiceCollection services, Action<WindowsServiceLifetimeOptions> configure)
{
#if !NETFRAMEWORK
Debug.Assert(RuntimeInformation.IsOSPlatform(OSPlatform.Windows));
Copy link
Member

Choose a reason for hiding this comment

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

These don't seem all that valuable. According to 04442bf#r759693314 it's to workaround an issue in the analyzer. I wonder if that issue still exists?

Copy link
Contributor

@buyaa-n buyaa-n May 3, 2023

Choose a reason for hiding this comment

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

I wonder if that issue still exists?

Probably would still warn as I don't see any annotation or guard. Isn't it windows only? Then we might want to add [SupportedOSPlatformGuard("windows")] attribute to this type to suppress the warnings here and propagate the warning to the callers

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Add one minor comment. LGTM otherwise.

#if !NETFRAMEWORK
RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
#else
Environment.OSVersion.StartsWith("Microsoft Windows NT");
Copy link
Member

Choose a reason for hiding this comment

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

This should always return true anyway when running with NETFX. why we bother getting the OS version anyway?

@ViktorHofer ViktorHofer merged commit 32a16d0 into dotnet:main May 3, 2023
@ViktorHofer ViktorHofer deleted the AvoidRuntimeInformationPkgDependency branch May 3, 2023 06:11
@ghost ghost locked as resolved and limited conversation to collaborators Jun 2, 2023
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