Skip to content

Merge Project | Build the Common Project #3837

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

Open
benrr101 wants to merge 8 commits into
base: main
Choose a base branch
from
+132 −31
Open

Merge Project | Build the Common Project #3837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
871859c
Add reference to MSS for netcore to fix InvalidUdtException reference…
benrr101 Dec 4, 2025
8250468
SqlDependencyUtils.* are all netcore files, rename as such, and wrap …
benrr101 Dec 4, 2025
d253767
Set build output path
benrr101 Dec 5, 2025
477219d
Add OS-specific check for target frameworks
benrr101 Dec 5, 2025
70fa18f
Builds! Whoohooo!
benrr101 Dec 5, 2025
61978e1
Common project build targets
benrr101 Dec 5, 2025
946f41b
Add common project back to the solution build targets
benrr101 Dec 9, 2025
2e92020
Comments from PR
benrr101 Dec 10, 2025
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
53 changes: 53 additions & 0 deletions build2.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="Current" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<!-- Build Parameters ============================================== -->
<PropertyGroup>
<!-- Configuration: Which build configuration to build -->
<!-- Allowed values: Debug, Release -->
<!-- Default value: Debug -->
<Configuration Condition="'$(Configuration)' == ''">Debug</Configuration>

<!-- DotnetPath: Path to folder containing the `dotnet` binary. Override this if a -->
<!-- specific version (eg, x86) is required. Otherwise, this defaults to using the -->
<!-- dotnet binary in the PATH variable. The provided path should end with a `\` (or -->
<!-- `/`) character. Eg. C:\x86\ -->
<DotnetPath Condition="'$(DotnetPath)' == ''"></DotnetPath>
</PropertyGroup>

<!-- Imports ======================================================= -->
<Import Project="src/Directory.Build.props" />

<!-- Microsoft.Data.SqlClient Build Targets ======================== -->
<PropertyGroup>
<MdsProjectPath>$(RepoRoot)src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj</MdsProjectPath>
</PropertyGroup>

<!-- BuildMds: Builds all binaries for MDS -->
<Target Name="BuildMds" DependsOnTargets="BuildMdsUnix;BuildMdsWindows" />

<!-- BuildMdsUnix: Builds all unix-specific MDS binaries -->
<Target Name="BuildMdsUnix">
<PropertyGroup>
<DotnetCommand>
$(DotnetPath)dotnet build $(MdsProjectPath)
-p:Configuration=$(Configuration)
-p:TargetOs=Unix
</DotnetCommand>
<DotnetCommand>$([System.Text.RegularExpressions.Regex]::Replace($(DotnetCommand), "\s{2,}", " "))</DotnetCommand>
Copy link
Contributor

@mdaigle mdaigle Dec 11, 2025

Choose a reason for hiding this comment

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

What does this line do?

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

This was a slight modification on a @paulmedynski special that replaced newlines with spaces. I went one further and had it convert >1 whitespace into a single space. This mostly just lets us write the command in multiple lines while dumping it to the logs as a single line.

Copy link
Contributor

@paulmedynski paulmedynski Dec 12, 2025

Choose a reason for hiding this comment

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

The <Exec> fails when the command text contains newlines. Maybe that's just on Windows - I don't remember entirely. This approach makes the command readable for humans, reasonable to log, and possible to execute :)

Copy link
Contributor

@mdaigle mdaigle Dec 12, 2025

Choose a reason for hiding this comment

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

Sounds good. I'd just request that we add a comment explaining it.

</PropertyGroup>
<Exec ConsoleToMsBuild="true" Command="$(DotNetCommand)" />
Copy link
Contributor

@paulmedynski paulmedynski Dec 11, 2025

Choose a reason for hiding this comment

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

Should we emit the command as a <Message> like build.proj sometimes does?

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

Your call, I kinda skipped it for now because it was a bit messier. I can add those 👍

Copy link
Contributor

@paulmedynski paulmedynski Dec 12, 2025

Choose a reason for hiding this comment

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

It helped with the long test run commands in build.proj. These commands aren't that long, but probably still helpful to emit as normal-priority <Message>s

</Target>

<!-- BuildMdsWindows: Builds all windows-specific MDS binaries -->
<Target Name="BuildMdsWindows">
<PropertyGroup>
<DotnetCommand>
$(DotnetPath)dotnet build $(MdsProjectPath)
-p:Configuration=$(Configuration)
-p:TargetOs=Windows
</DotnetCommand>
<DotnetCommand>$([System.Text.RegularExpressions.Regex]::Replace($(DotnetCommand), "\s{2,}", " "))</DotnetCommand>
</PropertyGroup>
<Exec ConsoleToMsBuild="true" Command="$(DotNetCommand)" />
</Target>
</Project>
8 changes: 7 additions & 1 deletion src/Microsoft.Data.SqlClient.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Microsoft Visual Studio Solution File, Format Version 12.00
Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio Version 17
VisualStudioVersion = 17.0.31912.275
MinimumVisualStudioVersion = 10.0.40219.1
Expand Down Expand Up @@ -561,6 +561,12 @@ Global
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Release|Any CPU.ActiveCfg = Release|Any CPU
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Release|x64.ActiveCfg = Release|Any CPU
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Release|x86.ActiveCfg = Release|Any CPU
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Debug|Any CPU.Build.0 = Debug|Any CPU
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Debug|x64.Build.0 = Debug|Any CPU
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Debug|x86.Build.0 = Debug|Any CPU
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Release|Any CPU.Build.0 = Release|Any CPU
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Release|x64.Build.0 = Release|Any CPU
{9A8996A8-6484-4AA7-B50F-F861430EDE2F}.Release|x86.Build.0 = Release|Any CPU
{4461063D-2F2B-274C-7E6F-F235119D258E}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{4461063D-2F2B-274C-7E6F-F235119D258E}.Debug|Any CPU.Build.0 = Debug|Any CPU
{4461063D-2F2B-274C-7E6F-F235119D258E}.Debug|x64.ActiveCfg = Debug|x64
Expand Down
8 changes: 4 additions & 4 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,11 +815,11 @@
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlDependencyUtils.cs">
<Link>Microsoft\Data\SqlClient\SqlDependencyUtils.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlDependencyUtils.AppDomain.cs">
<Link>Microsoft\Data\SqlClient\SqlDependencyUtils.AppDomain.cs</Link>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlDependencyUtils.AppDomain.netcore.cs">
<Link>Microsoft\Data\SqlClient\SqlDependencyUtils.AppDomain.netcore.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlDependencyUtils.AssemblyLoadContext.cs">
<Link>Microsoft\Data\SqlClient\SqlDependencyUtils.AssemblyLoadContext.cs</Link>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlDependencyUtils.AssemblyLoadContext.netcore.cs">
<Link>Microsoft\Data\SqlClient\SqlDependencyUtils.AssemblyLoadContext.netcore.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlEnclaveAttestationParameters.Crypto.cs">
<Link>Microsoft\Data\SqlClient\SqlEnclaveAttestationParameters.Crypto.cs</Link>
Expand Down
65 changes: 50 additions & 15 deletions src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>net462;net8.0;net9.0</TargetFrameworks>
<!-- General Properties ============================================== -->
<PropertyGroup>
<Configurations>Debug;Release;</Configurations>
Copy link
Contributor

@paulmedynski paulmedynski Dec 11, 2025

Choose a reason for hiding this comment

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

I don't see this property documented - what do we think it's doing?

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

I thought it was defining the allowed configurations that the project can be built in. But... yeah, I don't see it in the docs anywhere. I will remove it.

<RootNamespace />
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<RootNamespace />
</PropertyGroup>

<!-- OS Constants ==================================================== -->
<PropertyGroup>
<!-- @TODO: Move to directory.build.props? -->
<!-- If a target OS was not specified, use the current OS as the target OS. -->
<TargetOs Condition="'$(TargetOs)' == ''">$(OS)</TargetOs>
Copy link
Contributor

@paulmedynski paulmedynski Dec 11, 2025

Choose a reason for hiding this comment

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

Would it be better to just default to WINDOWS_NT so we get the most build coverage?

I think I would prefer that we build the same set of stuff by default regardless of where the build is occurring, and document that. Then, if you want to build a different set of stuff, you have to explicitly ask by setting TargetOs.

I find it surprising that the host OS dictates what to build by default.

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

I'm not so sure 🤔 Couple points:

  • This line exists mostly to ensure proper default behavior in an IDE, and by extension what version to run when executing/debugging tests within the IDE. Changing that to Windows_NT would require a developer to edit the csproj anytime they want to run tests in unix from their IDE - I think that's a much worse experience than defaulting to the host OS.
  • I left behind some simple to use lines below this that allow you to override the behavior in your IDE. This is mostly because IDEs usually won't syntax highlight code that has been excluded via #if. So the simple override allows a developer to get a rich editing experience for, say, unix on windows. It's nicely documented, too, imo.
  • By default, dotnet only defines its built-in OS constants based on the host OS. This is why we had to create an alternative set of constants and this TargetOs property. So there is precedent within the dotnet ecosystem to have the host OS dictate what is built.
  • For builds outside of the IDE, you still get a reasonable experience.
    • Using build2.proj, BuildMds builds both unix and windows in a single command
    • Using build2.proj, you also have separate unix and windows commands
    • Using dotnet on the csproj, you get your host os by default (which, as per above, is the precedent for dotnet), but you can easily supply TargetOs to force one or the other
    • Building the sln - that's the only non-idea situation, but that's mostly because I haven't fully fleshed it out.

Copy link
Contributor

@paulmedynski paulmedynski Dec 12, 2025

Choose a reason for hiding this comment

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

My hope is that I can eventually run this command anywhere and it builds everything:

$ dotnet build src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.sln

(I also hope we can come up with a better directory structure, but I digress.)

For now, we can't build everything due to the TargetOs property that includes files tailored for that OS, and excludes files tailored for other OSes. Fair enough, we need to choose a reasonable default to avoid having to always specify TargetOs on the command-line and in IDEs.

If I want to specify TargetOs on the command-line, it's not hard:

$ dotnet build .../Microsoft.Data.SqlClient.sln -p:TargetOs=WHATEVER

What is the IDE equivalent for easily choosing what TargetOs to specify when building or running tests for a project? Don't IDEs have a way for you to create whatever build/run configurations you want, setting whatever properties you want, and then it's a simple drop-down chooser or button? I'd be surprised if you need to edit the .csproj files to accomplish this.

Regardless, if you tell me that it's "normal" for dotnet projects to use the host OS to make build-time decisions, then I guess I'll shrug and say "That's weird", but we need to pick something so it is what it is.

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

For your first point - I can appreciate what you're hoping to do, and I'm mostly on board with it. For this work, I'm coming from a background of node.js project.json file - you have scripts that can be ran to do the tasks you want - in this sense our build.proj is filling the gap nicely. I'm not sure sln files have as much flexibility. In my head I separate it into: dev/IDE work correlates to sln, command line/pipeline work correlates to build.proj.

As for specifying OS in the IDE ... there isn't a great solution to that. There's two main knobs to twiddle for "configuration": build configuration and run targets. Build configurations are the "Release | Any CPU" format you're used to seeing, run targets are more IDE-specific saved entry points (eg, any exe files, test projects, etc) with whatever custom settings (eg, working directory, command line params). Build configuration is what we'd likely want to twiddle to do what you are looking for here.

The unfortunate part is this is (imo) clumsy way to deal with it. Basically, we'd need to define a new configuration in the sln, one for each os/configuration combo (ie: Debug-Unix, Debug-Windows, Release-Unix, Release-Windows). We'd also need to define these new configurations in the project itself, and do more string manipulation to extract the OS from the config... yada yada yada. It would work, and you'd get the it to show up in the list. Proof of concept:
image

I think the behavior that the host OS dictates what is built is sort of a holdover from early netframework days - you only built for windows, and VS only exists for windows, so why would you want anything different? If you use VS, you're using windows! But I also see it as the default behavior should be that - if I click the big "debug" button in the toolbar, I expect it to build and run the version that works on my computer. And there's the rub - the "default" configuration comes from the sln file, which doesn't support (afaik) switches based on OS. So, sure we can set Debug-Windows as the default, but if you're on unix, you'll get a surprise when you click run.

On the other hand, you'll have a two click way to change the OS you're targeting in your IDE on the fly. The alternative is uncomment a line in the csproj.
On the other other hand, you'd need to specify the OS in the configuration everytime you run from the command line. Which is a big departure from the expected values "Release" and "Debug".

I'm not sure what's better - what do you think? 😄


Expand All @@ -15,41 +17,74 @@
<!--<TargetOs>Windows_NT</TargetOs>-->

<!-- NOTE: These constants are prefixed with _ to keep them separate from .NET 5+ precompiler -->
<!-- flags. Those only apply to OS-specific target frameworks, and would interfere here. -->
<DefineConstants Condition="'$(TargetOs.ToUpper())' == 'UNIX'">$(DefineConstants),_UNIX</DefineConstants>
<DefineConstants Condition="'$(TargetOs.ToUpper())' == 'WINDOWS_NT'">$(DefineConstants),_WINDOWS</DefineConstants>
<!-- flags. Those only apply to OS-specific target frameworks, and would interfere here. -->
<DefineConstants Condition="'$(TargetOs.ToUpper())' == 'UNIX'">$(DefineConstants);_UNIX</DefineConstants>
Copy link
Contributor

@paulmedynski paulmedynski Dec 11, 2025

Choose a reason for hiding this comment

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

Instead of remembering to ToUpper() the TargetOs all the time, why not just make another property with the upper-case value and use that everywhere?

<TargetOs Condition="'$(TargetOs)' == ''">$(OS)</TargetOs>
<CanonicalTargetOs>$(TargetOs.ToUpper())</CanonicalTargetOs>
...
<DefineConstants Condition="'$(TargetOsCanonical' == UNIX'">$(DefineConstants);_UNIX</DefineConstants>

<DefineConstants Condition="'$(TargetOs.ToUpper())' == 'WINDOWS_NT'">$(DefineConstants);_WINDOWS</DefineConstants>
</PropertyGroup>

<!-- Target Frameworks =============================================== -->
<PropertyGroup>
<!-- net462 is only supported on Windows, so we will only add it if we're building for Windows -->
<TargetFrameworks>net8.0;net9.0</TargetFrameworks>
<TargetFrameworks Condition="'$(TargetOs.ToUpper())' == 'WINDOWS_NT'">$(TargetFrameworks);net462</TargetFrameworks>
</PropertyGroup>

<!-- Build Output ==================================================== -->
<PropertyGroup>
<!-- @TODO: Move to directory.build.props? -->
<ArtifactPath>$(RepoRoot)artifacts/</ArtifactPath>

<!-- MSBuild will add the target framework to the end of this path by default. Telling it not -->
<!-- to is possible but requires also specifying the IntermediateOutputPath. So while it -->
<!-- would be nice to have a flat directory structure, it's more hassle than its worth. -->
<OutputPath>$(ArtifactPath)$(AssemblyName)/$(Configuration)/$(TargetOs.ToLower())/</OutputPath>
Copy link
Contributor

@paulmedynski paulmedynski Dec 11, 2025

Choose a reason for hiding this comment

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

Why are we choosing ToLower() here for the TargetOs, but ToUpper() for the above checks?

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

Because the checks are independent of the formatting of the output path? 🤷‍♂️
Idk, there's some precedence for names like net6.0-winxyz, so I went with that. And idk, just seems more common to normalize cases for comparison with upper case.

How important of a change is it? 😉

Copy link
Contributor

@paulmedynski paulmedynski Dec 12, 2025

Choose a reason for hiding this comment

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

I'd be fine with upper-case in the path names if it eliminates an inconsistency. Or you can document it to call out why it's lower-case here and upper-case elsewhere.

</PropertyGroup>

<!-- Embedded resources ============================================== -->
<ItemGroup>
<!-- Linker directives to replace UseManagedNetworking with a constant if consumer specifies -->
<!-- the app context switch in their csproj. This file only applies to netcore on windows. -->
<!-- This file does not support pre-processor directives, so it must be conditionally -->
<!-- included into the build. -->
<EmbeddedResource Include="Resources/ILLink.Substitutions.xml"
Condition="'$(TargetOs.ToUpper())' == 'WINDOWS_NT'" />
Condition="'$(TargetOs.ToUpper())' == 'WINDOWS_NT' AND '$(TargetFramework)' != 'net462'" />
</ItemGroup>

<!-- References ====================================================== -->
<!-- References that apply to all target frameworks -->
<ItemGroup>
<!-- References that apply to all target frameworks -->
<PackageReference Include="Azure.Core" />
<PackageReference Include="Azure.Identity" />
<PackageReference Include="Microsoft.Bcl.Cryptography" />
<PackageReference Include="Microsoft.Extensions.Caching.Memory" />
<PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" />
<PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" />
<PackageReference Include="System.Configuration.ConfigurationManager" />
<PackageReference Include="System.Security.Cryptography.Pkcs" />
</ItemGroup>

<!-- References that only apply to net462 -->
<ItemGroup Condition="'$(TargetFramework)' == 'net462'">
<!-- References that only apply to net462 -->
<Reference Include="System.Configuration" />
<Reference Include="System.EnterpriseServices" />
<Reference Include="System.Transactions" />
Copy link
Contributor

@mdaigle mdaigle Dec 11, 2025

Choose a reason for hiding this comment

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

System.Text.Json PackageReference is missing here. Also missing a number of References.

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

The system.text.json override was explicitly removed in #3686

There are a few others that are missing ... notably SNI. I will investigate those. But it's worth noting that CI builds are passing 🤷‍♂️

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

OH DUH, yeah we're not even building it in the CI builds 🤦‍♂️
Uhm, but yeah, it all works locally for me ... I will still investigate

Copy link
Contributor

@mdaigle mdaigle Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

System.Text.Json was removed for .NET because it's included in the standard library, but we still need it to be package provided for .NET Framework.

<PackageReference Include="System.ValueTuple" />
<PackageReference Include="System.Threading.Channels" />
</ItemGroup>

<!-- System.Buffers, System.Memory, and System.Runtime.InteropServices.RuntimeInformation are included in .NET 10+ BCL -->
<ItemGroup Condition="$([MSBuild]::VersionLessThan($([MSBuild]::GetTargetFrameworkVersion($(TargetFramework))), '10.0'))">
<PackageReference Include="System.Buffers" />
<PackageReference Include="System.Memory" />
<PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" />
<!-- References that only apply to netcore -->
<ItemGroup Condition="'$(TargetFramework)' != 'net462'">
<PackageReference Include="Microsoft.SqlServer.Server" />
<PackageReference Include="System.Configuration.ConfigurationManager" />

<!-- References that only apply to netcore < 10 -->
<PackageReference Include="System.Buffers"
Condition="$([MSBuild]::VersionLessThan($([MSBuild]::GetTargetFrameworkVersion($(TargetFramework))), '10.0'))" />
Copy link
Contributor

@paulmedynski paulmedynski Dec 11, 2025

Choose a reason for hiding this comment

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

That's a mouthful. Maybe these should be in a separate <ItemGroup> to avoid duplicating this conditional 3 times.

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

They used to be ... but I was not a fan because it meant we had

<ItemGroup all platforms />
<ItemGroup netfx />
<ItemGroup netcore />
<ItemGroup netcore<10 />

My brain has trouble with these kinds of situations. See also the windows/netfx checks. Something something hierarchy being flattened and confusion about which group to a new item in. But there's no good rationale for it other than it just doesn't feel right. Which I know is a me issue, heh.

Ideally I'd like it to be:

<ItemGroup all platforms />
<ItemGroup netfx />
<ItemGroup netcore>
  <ItemGroup netcore<10 />
</ItemGroup>

But afaik msbuild doesn't support that.

... what if I flatten it into all the situations?

Copy link
Contributor Author

@benrr101 benrr101 Dec 12, 2025

Choose a reason for hiding this comment

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

Actually, it seems msbuild does support this. One moment please...

Copy link
Contributor

@paulmedynski paulmedynski Dec 12, 2025

Choose a reason for hiding this comment

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

It's definitely too bad you can't nest <ItemGroup> elements. I would choose the option that reduces duplication of complicated/dense instructions where subtle differences aren't obvious.

<PackageReference Include="System.Memory"
Condition="$([MSBuild]::VersionLessThan($([MSBuild]::GetTargetFrameworkVersion($(TargetFramework))), '10.0'))" />
<PackageReference Include="System.Runtime.InteropServices.RuntimeInformation"
Condition="$([MSBuild]::VersionLessThan($([MSBuild]::GetTargetFrameworkVersion($(TargetFramework))), '10.0'))" />
</ItemGroup>

<!-- CodeGen Targets ================================================= -->
<Import Project="$(ToolsDir)targets\GenerateThisAssemblyCs.targets" />
</Project>
25 changes: 14 additions & 11 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,24 +69,27 @@ internal static partial class ADP
internal const int MaxBufferAccessTokenExpiry = 600;

#region UDT
#if NETFRAMEWORK
private static readonly MethodInfo s_method = typeof(InvalidUdtException).GetMethod("Create", BindingFlags.NonPublic | BindingFlags.Static);
#endif

#if NETFRAMEWORK
private static readonly MethodInfo s_udtFactory =
typeof(InvalidUdtException).GetMethod("Create", BindingFlags.NonPublic | BindingFlags.Static);
#endif

/// <summary>
/// Calls "InvalidUdtException.Create" method when an invalid UDT occurs.
/// </summary>
internal static InvalidUdtException CreateInvalidUdtException(Type udtType, string resourceReasonName)
{
// @TODO: Can we adopt the netcore version?
InvalidUdtException e =
#if NETFRAMEWORK
(InvalidUdtException)s_method.Invoke(null, new object[] { udtType, resourceReasonName });
ADP.TraceExceptionAsReturnValue(e);
#else
InvalidUdtException.Create(udtType, resourceReasonName);
#endif
#if NETFRAMEWORK
InvalidUdtException e = (InvalidUdtException)s_udtFactory.Invoke(null, [udtType, resourceReasonName]);
#else
InvalidUdtException e = InvalidUdtException.Create(udtType, resourceReasonName);
#endif

TraceExceptionAsReturnValue(e);
return e;
}

#endregion

static private void TraceException(string trace, Exception e)
Expand Down
4 changes: 4 additions & 0 deletions ...SqlClient/SqlDependencyUtils.AppDomain.cs → ...t/SqlDependencyUtils.AppDomain.netcore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#if NET

using System;

namespace Microsoft.Data.SqlClient
Expand All @@ -18,3 +20,5 @@ partial void SubscribeToAppDomainUnload()
}
}
}

#endif
0 ...SqlDependencyUtils.AssemblyLoadContext.cs → ...dencyUtils.AssemblyLoadContext.netcore.cs
View file
Open in desktop
File renamed without changes.
Loading