Skip to content

Fix a few more API compat inconsistencies #68834

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.

Sign up for GitHub

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

Merged
ViktorHofer merged 1 commit into from
May 5, 2022
Merged

Fix a few more API compat inconsistencies #68834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessThread.FreeBSD.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ public TimeSpan TotalProcessorTime
/// Returns the amount of time the associated thread has spent running code
/// inside the application (not the operating system core).
/// </summary>
[UnsupportedOSPlatform("ios")]
[UnsupportedOSPlatform("tvos")]
[SupportedOSPlatform("maccatalyst")]
public TimeSpan UserProcessorTime
{
get
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,8 @@ public bool UseProxy
}

[UnsupportedOSPlatform("browser")]
[UnsupportedOSPlatform("ios")]
[UnsupportedOSPlatform("tvos")]
public IWebProxy? Proxy
{
get
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,9 @@ internal void HandleAltSvc(IEnumerable<string> altSvcHeaderValues, TimeSpan? res

if (!nextAuthorityPersist)
{
#if !ILLUMOS && !SOLARIS
Copy link
Member

@wfurt wfurt May 3, 2022

Choose a reason for hiding this comment

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

I'm not sure if we need to drag it here since neither OS is really supported.
cc: @am11

Copy link
Member

@am11 am11 May 3, 2022

Choose a reason for hiding this comment

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

If I understand it correctly, we were previously using UnknownUnix.cs pattern and not anymore (in favor of supported / unsupporter platform attributes). The compat tool is indicating that this call shouldn't be compiled on unknown unix.

Copy link
Member

@am11 am11 May 4, 2022

Choose a reason for hiding this comment

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

@ericstj, was this auto-generated? I know that we define TARGET_{OS} constants for OS specific projects, but I am not sure if we define {OS}.

runtime/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

Lines 30 to 44 in e109bbd

<PropertyGroup>
<DefineConstants Condition="'$(TargetsUnix)' == 'true'">$(DefineConstants);TARGET_UNIX</DefineConstants>
<DefineConstants Condition="'$(TargetsWindows)' == 'true'">$(DefineConstants);TARGET_WINDOWS</DefineConstants>
<DefineConstants Condition="'$(TargetsOSX)' == 'true'">$(DefineConstants);TARGET_OSX</DefineConstants>
<DefineConstants Condition="'$(TargetsMacCatalyst)' == 'true'">$(DefineConstants);TARGET_MACCATALYST</DefineConstants>
<DefineConstants Condition="'$(TargetsiOS)' == 'true'">$(DefineConstants);TARGET_IOS</DefineConstants>
<DefineConstants Condition="'$(TargetstvOS)' == 'true'">$(DefineConstants);TARGET_TVOS</DefineConstants>
<DefineConstants Condition="'$(TargetsBrowser)' == 'true'">$(DefineConstants);TARGET_BROWSER</DefineConstants>
<DefineConstants Condition="'$(TargetsAndroid)' == 'true'">$(DefineConstants);TARGET_ANDROID</DefineConstants>
<DefineConstants Condition="'$(TargetsLinux)' == 'true'">$(DefineConstants);TARGET_LINUX</DefineConstants>
<DefineConstants Condition="'$(TargetsFreeBSD)' == 'true'">$(DefineConstants);TARGET_FREEBSD</DefineConstants>
<DefineConstants Condition="'$(TargetsNetBSD)' == 'true'">$(DefineConstants);TARGET_NETBSD</DefineConstants>
<DefineConstants Condition="'$(Targetsillumos)' == 'true'">$(DefineConstants);TARGET_ILLUMOS</DefineConstants>
<DefineConstants Condition="'$(TargetsSolaris)' == 'true'">$(DefineConstants);TARGET_SOLARIS</DefineConstants>
</PropertyGroup>

Copy link
Member

@ViktorHofer ViktorHofer May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Those are added by the SDK here: https://github.com/dotnet/sdk/blob/997376c6bc798af22633a8eb36074a0acec8a200/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets#L201

image

Copy link
Member

@ViktorHofer ViktorHofer May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

System.Private.CoreLib doesn't use the TargetPlatform feature which requires platform defines to be manually specified.

Copy link
Member

@am11 am11 May 4, 2022

Choose a reason for hiding this comment

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

If some of our libraries manually pass in TARGETS_* define constants based on the target platform, then those should probably be cleaned-up (except for CoreLib).

Yup, there is #39174 tracking review of defined vs. used constants. We are using TARGET_{OS} and TARGET_{ARCH}, but then we also had examples of TARGETS_XXX (plural) and TargetsXxx. 😁

Copy link
Member Author

@ericstj ericstj May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The compat tool is indicating that this call shouldn't be compiled on unknown unix.

The compiler is actually flagging the illumos and solaris builds of S.N.Http as calling unsupported API. I chose these defines based on what I saw being passed into the compiler. I had to ifdef to avoid compiler warnings as error.

@am11 @wfurt can you please let me know if you'd like anything different here? I'm trying to keep this change as minimal as possible. I could just pragma suppress the compiler warnings. I chose not to do that since these APIs were actually throwing on the unsupported platforms and I didn't want to ignore that signal.

Copy link
Member

@am11 am11 May 4, 2022

Choose a reason for hiding this comment

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

Minimal change looks good to me. There are only a few places which require such conditions, the rest of places are filtered out via Supported/Unsupported OS attributes.

Copy link
Member

@wfurt wfurt May 5, 2022

Choose a reason for hiding this comment

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

I think it is ok. I wish we did not have to drag #ifs to the HTTP code but I understand the reasoning. It would be nice if the tool can simply ignore unfinished/incomplete targets like Solaris of FreeBSD.

Copy link
Member Author

@ericstj ericstj May 5, 2022

Choose a reason for hiding this comment

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

I was imagining a slightly larger refactor with partial files might also work here but I didn't want to make the diff that large.

_poolManager.StartMonitoringNetworkChanges();
#endif
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions ...aries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ internal sealed class HttpConnectionPoolManager : IDisposable
private readonly IWebProxy? _proxy;
private readonly ICredentials? _proxyCredentials;

#if !ILLUMOS && !SOLARIS
private NetworkChangeCleanup? _networkChangeCleanup;
#endif

/// <summary>
/// Keeps track of whether or not the cleanup timer is running. It helps us avoid the expensive
Expand Down Expand Up @@ -150,6 +152,7 @@ public HttpConnectionPoolManager(HttpConnectionSettings settings)
}
}

#if !ILLUMOS && !SOLARIS
/// <summary>
/// Starts monitoring for network changes. Upon a change, <see cref="HttpConnectionPool.OnNetworkChanged"/> will be
/// called for every <see cref="HttpConnectionPool"/> in the <see cref="HttpConnectionPoolManager"/>.
Expand Down Expand Up @@ -219,6 +222,7 @@ public void Dispose()
GC.SuppressFinalize(this);
}
}
#endif

public HttpConnectionSettings Settings => _settings;
public ICredentials? ProxyCredentials => _proxyCredentials;
Expand Down Expand Up @@ -454,7 +458,9 @@ public void Dispose()
pool.Value.Dispose();
}

#if !ILLUMOS && !SOLARIS
_networkChangeCleanup?.Dispose();
#endif
}

/// <summary>Sets <see cref="_cleaningTimer"/> and <see cref="_timerIsRunning"/> based on the specified timeout.</summary>
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/System.Net.NetworkInformation/ref/System.Net.NetworkInformation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,11 @@ public partial class NetworkChange
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
[System.ObsoleteAttribute("This API supports the .NET Framework infrastructure and is not intended to be used directly from your code.", true)]
public NetworkChange() { }
[System.Runtime.Versioning.UnsupportedOSPlatform("illumos")]
[System.Runtime.Versioning.UnsupportedOSPlatform("solaris")]
public static event System.Net.NetworkInformation.NetworkAddressChangedEventHandler? NetworkAddressChanged { add { } remove { } }
[System.Runtime.Versioning.UnsupportedOSPlatform("illumos")]
[System.Runtime.Versioning.UnsupportedOSPlatform("solaris")]
public static event System.Net.NetworkInformation.NetworkAvailabilityChangedEventHandler? NetworkAvailabilityChanged { add { } remove { } }
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
[System.ObsoleteAttribute("This API supports the .NET Framework infrastructure and is not intended to be used directly from your code.", true)]
Expand Down
5 changes: 5 additions & 0 deletions ...stem.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkAddressChange.OSX.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
using Microsoft.Win32.SafeHandles;
using System.Diagnostics;
using System.Threading;
Expand Down Expand Up @@ -38,6 +39,8 @@ public partial class NetworkChange
private static readonly AutoResetEvent s_runLoopStartedEvent = new AutoResetEvent(false);
private static readonly AutoResetEvent s_runLoopEndedEvent = new AutoResetEvent(false);

[UnsupportedOSPlatform("illumos")]
[UnsupportedOSPlatform("solaris")]
public static event NetworkAddressChangedEventHandler? NetworkAddressChanged
{
add
Expand Down Expand Up @@ -75,6 +78,8 @@ public static event NetworkAddressChangedEventHandler? NetworkAddressChanged
}
}

[UnsupportedOSPlatform("illumos")]
[UnsupportedOSPlatform("solaris")]
public static event NetworkAvailabilityChangedEventHandler? NetworkAvailabilityChanged
{
add
Expand Down
5 changes: 5 additions & 0 deletions ...tem.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkAddressChange.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics;
using System.Net.Sockets;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
using System.Threading;
using System.Threading.Tasks;

Expand Down Expand Up @@ -43,6 +44,8 @@ private static Socket? Socket
private static Timer? s_availabilityTimer;
private static bool s_availabilityHasChanged;

[UnsupportedOSPlatform("illumos")]
[UnsupportedOSPlatform("solaris")]
public static event NetworkAddressChangedEventHandler? NetworkAddressChanged
{
add
Expand Down Expand Up @@ -83,6 +86,8 @@ public static event NetworkAddressChangedEventHandler? NetworkAddressChanged
}
}

[UnsupportedOSPlatform("illumos")]
[UnsupportedOSPlatform("solaris")]
public static event NetworkAvailabilityChangedEventHandler? NetworkAvailabilityChanged
{
add
Expand Down
5 changes: 5 additions & 0 deletions ....Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkAddressChange.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

using System.Collections.Generic;
using System.Net.Sockets;
using System.Runtime.Versioning;
using System.Threading;

namespace System.Net.NetworkInformation
Expand All @@ -13,6 +14,8 @@ public partial class NetworkChange
{
private static readonly object s_globalLock = new object();

[UnsupportedOSPlatform("illumos")]
[UnsupportedOSPlatform("solaris")]
public static event NetworkAvailabilityChangedEventHandler? NetworkAvailabilityChanged
{
add
Expand All @@ -25,6 +28,8 @@ public static event NetworkAvailabilityChangedEventHandler? NetworkAvailabilityC
}
}

[UnsupportedOSPlatform("illumos")]
[UnsupportedOSPlatform("solaris")]
public static event NetworkAddressChangedEventHandler? NetworkAddressChanged
{
add
Expand Down