Merge | OS Flags #3810

benrr101 · 2025-11-26T00:34:42Z

Description

Introduced _WINDOWS/_UNIX compile-time constants and TargetOs selection in src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj, plus mirrored updates to netcore/src/Microsoft.Data.SqlClient.csproj and netfx/src/Microsoft.Data.SqlClient.csproj so OS-gated code can be compiled from the common project and Windows-only ILLink substitutions are conditionally embedded (Resources/ILLink.Substitutions.xml).

Normalized OS-specific sources to .windows/.unix naming and wired them into the new flags (e.g., LocalDbApi., LocalDB.netcore., PacketHandle., SessionHandle., SqlFileStream., SqlColumnEncryption, TdsParser*, SNI interop files), with new netfx-specific shims like PacketHandle.netfx.cs and SessionHandle.netfx.cs. Refactored shared helpers into OS- and target-aware partials: AdapterUtil.cs now delegates netfx- and netcore-specific pieces to AdapterUtil.netfx.cs/AdapterUtil.netcore.cs; DbConnectionPoolIdentity.cs and SqlDataSourceEnumerator.cs now pick managed vs native implementations based on _WINDOWS/_UNIX and LocalAppContextSwitches.UseManagedNetworking; added netfx-only System/Diagnostics/CodeAnalysis.netfx.cs.

Folded the netcore app–specific SSL over TDS stream logic into ManagedSni/SslOverTdsStream.netcore.cs, adding span-based sync/async read handling and packet encapsulation directly in the shared file.

Tidied LocalAppContextSwitches.cs for clearer NET vs NETFRAMEWORK behavior (globalization invariant handling and managed SNI defaults) under the new OS constants.

[The above is AI generated]

Issues

Continuation of work from #1261

Testing

Projects still build, and the common project is building too 👀

…o methods that had separate implementations were identical...

…partial. This file is too big to come up with a consistent way to split all this stuff up.

… file

…c class. Cleaned up some code (removed redundant parentheses, split some long lines, removed always-false variables)

…etHandle.netfx.

…ssionHandle.netfx. This really makes these classes easier to read.

…pective #if's

…witches

… add comment explaining why this file exists.

…indicate that it only applies to Windows.

…ogical paths match the actual path

Copilot

Pull request overview

This PR introduces compile-time OS constants (_WINDOWS/_UNIX) to consolidate platform-specific code across the Microsoft.Data.SqlClient project. The refactoring normalizes file naming conventions to .windows/.unix suffixes, eliminates separate platform-specific partial files in favor of conditional compilation within shared files, and updates project files to support cross-platform builds from a unified source tree. Key changes include merging AdapterUtil.Windows.cs/AdapterUtil.Unix.cs into the main file with OS guards, consolidating DbConnectionPoolIdentity platform variants, and folding .NET Core app-specific SSL stream logic into the main implementation.

Key Changes:

Introduced _WINDOWS and _UNIX preprocessor constants based on TargetOs in project files
Normalized OS-specific file naming from .Windows/.Unix to .windows/.unix
Consolidated platform-specific partial classes into shared files with #if directives
Refactored PacketHandle and SessionHandle to use platform-specific files with improved documentation

Reviewed changes

Copilot reviewed 84 out of 86 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
System/Diagnostics/CodeAnalysis.netfx.cs	Extracted NETFRAMEWORK-only attributes to separate file
System/Diagnostics/CodeAnalysis.cs	Removed file (content moved to .netfx variant)
Resources/ILLink.Substitutions.xml	Changed from Unix-specific to Windows-specific with feature flags
Resources/ILLink.Substitutions.Unix.xml	Removed (consolidated into main file with OS conditions)
SqlFileStream.windows.cs	Added `_WINDOWS` guard and TODO comment
TdsParser*.windows.cs	Added `_WINDOWS` guards and formatting improvements
PacketHandle/SessionHandle variants	Created platform-specific shims with improved documentation
SslOverTdsStream files	Merged NetCoreApp.cs into netcore.cs with all I/O methods
AdapterUtil.cs + variants	Consolidated Windows/Unix partials into main file with OS guards
DbConnectionPoolIdentity.cs	Merged Windows/Unix variants with conditional logic
SqlDataSourceEnumerator.cs	Unified platform selection logic in main file
LocalAppContextSwitches.cs	Added NETFRAMEWORK GlobalizationInvariantMode stub, clarified OS-specific UseManagedNetworking
Project files	Added TargetOs properties and _WINDOWS/_UNIX constant definitions

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolIdentity.cs

edwardneal

Very early review here, but the comment in SqlColumnEncryptionCspProvider has wider implications - how much of the work needs to be done at compile time? Could we replace the conditional compilations with runtime OS checks?

If that works then we could eliminate the need to build and package for multiple OSes completely.

edwardneal · 2025-11-26T07:05:15Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

+    <DefineConstants Condition="'$(OSGroup.ToUpper())'=='UNIX'">_UNIX</DefineConstants>
+    <DefineConstants Condition="'$(OSGroup.ToUpper())'=='WINDOWS_NT'">_WINDOWS</DefineConstants>


There are likely to be other points to untangle, but netcore automatically defines the appropriate WINDOWS preprocessor symbol. It might be better to polyfill this in the netfx project rather than define a new one.

While true, as far as I know, there's no way to override those. We want to be able to build unix on windows and windows on unix (and to a lesser extent want to be able to get IDE highlighting for one on the other), so we need a flag that can be overridden. This seems like the best way to do it, as per the internet :)

edwardneal · 2025-11-26T07:07:28Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

-      <Link>Resources\ILLink.Substitutions.Unix.xml</Link>
+    <EmbeddedResource Include="$(CommonSourceRoot)Resources\Microsoft.Data.SqlClient.SqlMetaData.xml">
+      <Link>Resources\Microsoft.Data.SqlCLient.SqlMetaData.xml</Link>
+      <LogicalName>Microsoft.Data.SqlClient.SqlMetaData.xml</LogicalName> <!-- @TODO: How necessary is this? -->


It's not necessary, the code to replace it is just bulky - #3372

Haha, I was specifically calling out the logical name tag, but yeah if we can junk the xml file altogether I'd approve of it.

edwardneal · 2025-11-26T07:08:48Z

src/Microsoft.Data.SqlClient/src/Interop/Windows/Kernel32/Kernel32Safe.netfx.cs

 // See the LICENSE file in the project root for more information.

-#if NETFRAMEWORK
+#if NETFRAMEWORK && _WINDOWS


Doesn't NETFRAMEWORK imply Windows-only code?
(here and elsewhere)

yeah, probably ... I'm a bit torn on these. I think no matter which way it's done, it's a little confusing.

edwardneal · 2025-11-26T07:17:34Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.netcore.unix.cs

+// @TODO: Introduce polymorphism to remove need for this level of indirection
+// @TODO: Also, why do we have any other type besides managed defined here?


PacketHandles are used to transport either native or managed packets around TdsParserStateObject.

I know we discussed this before, but I'm still not convinced ... so I left these comments to consider it further.
But considering on unix, we never have native packets, I don't know why we need to have an object with both types defined.

edwardneal · 2025-11-26T07:20:07Z

src/Microsoft.Data.SqlClient/src/Resources/ILLink.Substitutions.Unix.xml

-      <!-- The Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows switch must always be true on Unix platforms. -->
-      <method signature="System.Boolean get_UseManagedNetworking()" body="stub" value="true" />


This is different to the Windows ILLink.Substitutions.xml resource, and shouldn't be disregarded.

Yes, but all it does is hard code UseManagedNetworking to true on unix - which at the time wasn't defined in the app context switches. We can do that with code that can be switched on/off via pre-compiler flags and eliminate a file that would confuse the heck out of someone unfamiliar with it.

The original reason for having two files was to block someone from switching off the managed SNI on Linux using the below RuntimeHostConfigurationOption in their client application's csproj when trying to publish in a trimmed environment:

<RuntimeHostConfigurationOption Include="Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows" value="false" />

On second thoughts though, I don't think the use of the Windows ILLink.Substitutions.xml resource will cause a problem - it's just a guardrail on invalid usage of the library, trimming isn't technically supported yet, and this could be wrapped up in a runtime-level OS check.

benrr101 · 2025-11-26T18:45:35Z

@edwardneal As to your overall comment that it would be nice to eliminate the compile-time checks: yes, yes it would be nice :) @paulmedynski and I chatted about it and I think we came to the conclusion that this was a decent enough solution for now. A lot exists because we have native SNI - and it'd be nice if we can deprecate native SNI.

paulmedynski · 2025-11-26T19:02:35Z

We will be auditing the compile-time checks (not as part of this PR) to see how many of them could become inexpensive runtime checks (i.e. checked once per runtime, or "not often"). If the runtime costs are very low, then we would consider converting them in order to gain project architecture efficiencies and eliminate a large chunk of the build setup that exists to support OS-specific artifacts.

edwardneal · 2025-11-26T22:53:16Z

Thanks - that sounds good. The JIT will lift the OS-level checks via OperatingSystem.IsWindows in Release configuration, so they're very cheap and probably don't need to be cached (see SharpLab examples for netcore and netfx.)

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs

Copilot

Pull request overview

Copilot reviewed 81 out of 83 changed files in this pull request and generated no new comments.

…at are platform specific in Fix case-sensitivity issues in netfx project

Copilot

Pull request overview

Copilot reviewed 83 out of 85 changed files in this pull request and generated no new comments.

benrr101 added 30 commits November 21, 2025 12:49

Add pre-processor constants for target OS

01cfa53

Add #if _WINDOWS to the Interop/Windows files

1defe9f

Split out the netfx methods and junk from AdapterUtil

725928a

Split netcore junk from AdapterUtil into a separate file. Oh wait, tw…

5e5064e

…o methods that had separate implementations were identical...

Merge the unix and windows version of AdapterUtil back into the main …

f5e7ac9

…partial. This file is too big to come up with a consistent way to split all this stuff up.

Consolidate windows/unix versions of SqlDataSourceEnumerator into one…

0639587

… file

Merging DbConnectionPoolIdentity os-specific versions into the generi…

cd0158e

…c class. Cleaned up some code (removed redundant parentheses, split some long lines, removed always-false variables)

Add os #if wrappers to LocalDbApi

61fa101

Apply #if wrappers to LocalDB files

9908a50

Merge the SslOverTdsStream files together

a2043d3

Split PacketHandle.Windows into PacketHandle.netcore.windows and Pack…

5cd3bd2

…etHandle.netfx.

Wrap PacketHandle.netcore.unix in _UNIX

8726ed6

Split Sessionhandle.Windows into SessionHandle.netcore.Windows and Se…

dc96b5e

…ssionHandle.netfx. This really makes these classes easier to read.

Wrap SessionHandle.netcore.unix in _UNIX flag

02fbdef

Wrap SqlColumnEncryptionCngProvider in _UNIX and _WINDOWS

0c765f8

Wrap SqlColumnEncryptionCspProvider in _UNIX and _WINDOWS

d19082e

Wrap TdsParserSafeHandles.windows in _WINDOWS

e44a384

Add _UNIX wrapper to TdsParserStateObjectFactory.unix.cs

8e3d683

Wrap TdsParseStateObject[Managed.netcore|Native.windows] in their res…

6a66804

…pective #if's

Wrap SqlFileStream files in appropriate #if

c308c11

Remove ILLink.Substitutions.Unix.xml and embed it in LocalAppContextS…

047dfc4

…witches

Conditionally include ILLink.Substitutions.xml for windows *sigh*

e296c95

Make CodeAnalysis be a netfx-only file, remove unused attributes, and…

9ad3a35

… add comment explaining why this file exists.

Rename SqlBatchCommand.Net8OrGreater.cs to SqlBatchCommand.netcore.cs

5674ac0

Wrap SqlDataSourceEnumeratorNativeHelper.cs in windows and rename to …

cffa6d5

…indicate that it only applies to Windows.

Bring in all files for the netcore project, re-sort them, make sure l…

811406f

…ogical paths match the actual path

Netcore project embedded resources

42cecd8

Get rid of TargetsWindows/TargetsUnix

a907dc9

Bring in all files for the netcore project, re-sort them, make sure l…

cc0a6ee

…ogical paths match the actual path

Resource files in netfx project

166ff48

Copilot AI review requested due to automatic review settings November 26, 2025 00:34

Copilot started reviewing on behalf of benrr101 November 26, 2025 00:35 View session

Copilot finished reviewing on behalf of benrr101 November 26, 2025 00:36

Copilot AI reviewed Nov 26, 2025

View reviewed changes

edwardneal reviewed Nov 26, 2025

View reviewed changes

benrr101 added 3 commits November 26, 2025 13:35

First round of PR comments

2576df7

Fix file inclusion

deda198

Fix "unreachable code detected" issue in unix build

6682db1

benrr101 changed the title ~~Dev/russellben/os flags~~ Merge | OS Flags Nov 26, 2025

paulmedynski requested changes Nov 27, 2025

View reviewed changes

paulmedynski self-assigned this Nov 27, 2025

Merge branch 'main' into dev/russellben/os-flags

615c30c

Copilot AI review requested due to automatic review settings December 1, 2025 19:30

Copilot started reviewing on behalf of benrr101 December 1, 2025 19:30 View session

Copilot finished reviewing on behalf of benrr101 December 1, 2025 19:31

Copilot AI reviewed Dec 1, 2025

View reviewed changes

Addressing comments from @paulmedynski

d8a4c47

paulmedynski previously approved these changes Dec 1, 2025

View reviewed changes

benrr101 modified the milestones: 7.0.0-preview3, 7.0.0-preview4 Dec 2, 2025

Add OS constants to test common project, wrap app context switches th…

2e2de79

…at are platform specific in Fix case-sensitivity issues in netfx project

Copilot AI review requested due to automatic review settings December 2, 2025 19:45

benrr101 dismissed paulmedynski’s stale review via 2e2de79 December 2, 2025 19:45

Copilot started reviewing on behalf of benrr101 December 2, 2025 19:46 View session

Copilot finished reviewing on behalf of benrr101 December 2, 2025 19:46

paulmedynski approved these changes Dec 2, 2025

View reviewed changes

Copilot AI reviewed Dec 2, 2025

View reviewed changes

		<DefineConstants Condition="'$(OSGroup.ToUpper())'=='UNIX'">_UNIX</DefineConstants>
		<DefineConstants Condition="'$(OSGroup.ToUpper())'=='WINDOWS_NT'">_WINDOWS</DefineConstants>

		// @TODO: Introduce polymorphism to remove need for this level of indirection
		// @TODO: Also, why do we have any other type besides managed defined here?

		<!-- The Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows switch must always be true on Unix platforms. -->
		<method signature="System.Boolean get_UseManagedNetworking()" body="stub" value="true" />

Merge | OS Flags #3810

Are you sure you want to change the base?

Merge | OS Flags #3810

Uh oh!

Conversation

benrr101 commented Nov 26, 2025

Description

Issues

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edwardneal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edwardneal Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benrr101 commented Nov 26, 2025

Uh oh!

paulmedynski commented Nov 26, 2025

Uh oh!

edwardneal commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

edwardneal Nov 26, 2025 •

edited

Loading

edwardneal commented Nov 26, 2025 •

edited

Loading