Merge Project | Build the Common Project #3837

benrr101 · 2025-12-10T00:03:21Z

Description

This PR kicks the tires on building the common project. The goal of this PR was simply to get the common project to build, generate artifacts, and cause no disturbances to existing projects or pipelines. Here's a rundown of the changes:

General tweaks to get the common project to build at all
- Adding #if NET wrappers around SqlDependencyUtils.AppDomain.netcore.cs
- Adding references to packages missing from the common project but present in the netfx/netcore projects
Change target frameworks based on OS being built
- This may be marginally controversial - we've never had target frameworks be conditional before. However, not doing this causes issues when building on unix: files that are included only on windows builds will be exculded on unix, and the dependency chain goes much higher than just the conditionally included files. Thus, the easiest way to get around this for now is to just disable building netfx when building for unix. This can still be overridden by setting TargetOs to Windows_NT, enabling building netfx binaries on unix.
Introduce new artifacts paths
- This shouldn't be controversial, but might seem messy for a bit.
- You now have artifacts/Microsoft.Data.SqlClient/(configuration)/(targetos)/(targetframework)
  - I wanted to flatten the last two, but this isn't possible without mucking around with the obj folder path, and at that point it's too much complexity in the csproj.
- Obj folder path is default (ie, in the same folder as the project) b/c we do not need to make it that complicated
Introducing a build2.proj file
- This choice is more controversial. However, I'm doing it because I want to design as simple of a build procedure as possible, not beholden to existing variables, structures, etc. I also do not want to cause interference with the existing build.proj. And let's be real, build.proj is a mess - now is a good time for a fresh start.
- It provides three targets
  - BuildMds - builds common project for unix and windows
  - BuildMdsUnix - builds common project for unix
  - BuildMdsWindows - builds common project for windows
- It also use dotnet to do the build instead.

Testing

Project builds in IDE on windows and linux
Project builds from direct command line on windows and linux
Project builds from build2.proj command line on windows and linux

… issue

…in #if NET

Remove runtime identifier/target framework from output path

Copilot

Pull request overview

This PR establishes the foundation for building a unified "common" Microsoft.Data.SqlClient project that consolidates code from the platform-specific netfx and netcore projects. The changes enable conditional target framework selection based on the target OS (net462 only on Windows), introduce OS-aware artifact output paths, and provide new build targets via build2.proj. Key modifications include adding #if NET preprocessor directives to platform-specific files, refactoring UDT exception creation code, updating package references, and adjusting file references in the netcore project to point to renamed SqlDependency files with .netcore.cs extensions.

Conditional target frameworks: net462 only included when building for Windows
New artifact directory structure: artifacts/(AssemblyName)/(Configuration)/(TargetOs)/(TargetFramework)
build2.proj provides BuildMds, BuildMdsUnix, and BuildMdsWindows targets for streamlined builds

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
SqlDependencyUtils.AssemblyLoadContext.netcore.cs	New file with `#if NET` wrapper for AssemblyLoadContext-based unloading support
SqlDependencyUtils.AppDomain.netcore.cs	Added `#if NET` wrapper to conditionally compile AppDomain unload logic
AdapterUtil.cs	Refactored UDT exception creation, improved formatting, moved TraceExceptionAsReturnValue call outside conditional blocks
Microsoft.Data.SqlClient.csproj	Added conditional target frameworks, new artifact paths, package reference reorganization for netfx vs netcore
netcore/src/Microsoft.Data.SqlClient.csproj	Updated file references from `.cs` to `.netcore.cs` for SqlDependencyUtils files
Microsoft.Data.SqlClient.sln	Added Build.0 configurations for common project to enable building in Visual Studio
build2.proj	New build orchestration file with targets for building common project on Unix and Windows

build2.proj

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

edwardneal

No major comments, just a confirmation of Copilot and a quick naming nit.

The commentary about OS-specific handling comes at the same time as I've been looking at what kind of work would be needed to eliminate the need for it entirely. It looks like almost everything can be trivially lifted into runtime checks, with one or two benchmarks to be run for cases where Unix has a slightly different class structure. I'm hoping to start unwinding the need for this soon.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs

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

paulmedynski

Looking for some commentary on a few things, and some suggestions.

build2.proj

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

mdaigle

Mainly curious if it compiles for framework, because we seem to be missing a lot of references.

build2.proj

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

…ariable.

…et/sqlclient into dev/russellben/build-common-project ... working on two different machines

Copilot

Pull request overview

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

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs

build2.proj

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

mdaigle

Looks good overall. Want to take a close look at the dependencies, but doesn't need to happen in this PR.

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

cheenamalhotra · 2025-12-17T04:49:11Z

build2.proj

+	</Target>
+
+	<!-- BuildMdsWindows: Builds all windows-specific MDS binaries -->
+	<Target Name="BuildMdsWindows">


Don't forget strong name key signing is a MUST.

Ah, good catch. I will get that in on this PR. Otherwise, I'll definitely forget.

Is this planned for later?

benrr101 · 2025-12-17T19:46:34Z

@mdaigle Net462 is building happily - Can't guarantee it all works yet, but that's the goal of the next PR :)

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

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

paulmedynski

Looks pretty good! Just a couple of pedantic comments :)

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

build2.proj

cheenamalhotra · 2025-12-19T20:06:05Z

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

+  <ItemGroup Condition="'$(TargetFramework)' != 'net462'">
+    <PackageReference Include="Azure.Core" />
+    <PackageReference Include="Azure.Identity" />
+    <PackageReference Include="Microsoft.Bcl.Cryptography" />


Please also include dependency on Microsoft.Identity.client (MSAL).

It should be present in all projects, I don't recall why it got removed. But, we use its APIs directly in AAD implementation and cannot connect with AAD Auth modes when debugging locally unless its included.

cheenamalhotra · 2025-12-19T20:09:38Z

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

+  <!-- References for netcore -->
+  <ItemGroup Condition="'$(TargetFramework)' != 'net462'">
+    <PackageReference Include="Azure.Core" />
+    <PackageReference Include="Azure.Identity" />


Why not combine shared dependencies that must be included regardless of target framework?

cheenamalhotra · 2025-12-19T20:11:49Z

build2.proj

+				  -p:TargetOs=Windows_NT
+			</DotnetCommand>
+			<!-- Convert more than one whitespace character into one space --> 
+			<DotnetCommand>$([System.Text.RegularExpressions.Regex]::Replace($(DotnetCommand), "\s+", " "))</DotnetCommand>


What's this about? Why so much complexity?

cheenamalhotra · 2025-12-19T20:14:15Z

build2.proj

Build proj is outdated build system, I was honestly hoping for a better and modern build system we would use with the unified Dotnet SDK style project. Do we have options to consider?

benrr101 added 7 commits December 9, 2025 17:28

Add reference to MSS for netcore to fix InvalidUdtException reference…

871859c

… issue

SqlDependencyUtils.* are all netcore files, rename as such, and wrap …

8250468

…in #if NET

Set build output path

d253767

Add OS-specific check for target frameworks

477219d

Remove runtime identifier/target framework from output path

Builds! Whoohooo!

70fa18f

Common project build targets

61978e1

Add common project back to the solution build targets

946f41b

benrr101 added this to the 7.0.0-preview4 milestone Dec 10, 2025

Copilot AI review requested due to automatic review settings December 10, 2025 00:03

benrr101 requested a review from a team as a code owner December 10, 2025 00:03

benrr101 added the Common Project 🚮 Things that relate to the common project project label Dec 10, 2025

Copilot started reviewing on behalf of benrr101 December 10, 2025 00:03 View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

build2.proj Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Outdated Show resolved Hide resolved

edwardneal reviewed Dec 10, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Outdated Show resolved Hide resolved

priyankatiwari08 self-assigned this Dec 10, 2025

Comments from PR

2e92020

paulmedynski self-assigned this Dec 11, 2025

paulmedynski reviewed Dec 11, 2025

View reviewed changes

mdaigle reviewed Dec 11, 2025

View reviewed changes

build2.proj Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Outdated Show resolved Hide resolved

edwardneal mentioned this pull request Dec 13, 2025

Cleanup | Preparation for OS-independent builds #3844

Open

benrr101 added 2 commits December 16, 2025 13:16

Rework package references as per comments, add normalized target os v…

8d4ca14

…ariable.

Merge branch 'dev/russellben/build-common-project' of github.com:dotn…

4c25b0f

…et/sqlclient into dev/russellben/build-common-project ... working on two different machines

Copilot AI review requested due to automatic review settings December 16, 2025 19:22

Copilot started reviewing on behalf of benrr101 December 16, 2025 19:23 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

Add message on exec

2a0ff49

mdaigle previously approved these changes Dec 16, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Show resolved Hide resolved

cheenamalhotra reviewed Dec 17, 2025

View reviewed changes

Add strong name signing and triple check package references

ca43c20

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

benrr101 dismissed mdaigle’s stale review via ca43c20 December 17, 2025 19:45

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

Copilot AI reviewed Dec 17, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Show resolved Hide resolved

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Show resolved Hide resolved

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Show resolved Hide resolved

paulmedynski reviewed Dec 17, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Outdated Show resolved Hide resolved

build2.proj Outdated Show resolved Hide resolved

Unconditional assembly originator key file, clarify command

392702b

mdaigle approved these changes Dec 18, 2025

View reviewed changes

cheenamalhotra reviewed Dec 19, 2025

View reviewed changes

Merge Project | Build the Common Project #3837

Are you sure you want to change the base?

Merge Project | Build the Common Project #3837

Conversation

benrr101 commented Dec 10, 2025

Description

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!

edwardneal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdaigle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdaigle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cheenamalhotra Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

benrr101 Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

benrr101 commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cheenamalhotra Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!