Merge Project | Build the Common Project

[benrr101]

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

[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

[Copilot]

The condition for including ILLink.Substitutions.xml appears incorrect. Based on the code in LocalAppContextSwitches.cs, the UseManagedNetworking switch is only relevant for .NET Core (NET) on Windows, not for .NET Framework (net462). The condition should check for .NET Core target frameworks on Windows (e.g., '$(TargetFramework)' != 'net462') instead of checking == 'net462'. The comment on line 46 correctly states this file "only applies to netcore on windows", but the condition contradicts this.

Suggested change

	Condition="'$(TargetOs.ToUpper())' == 'WINDOWS_NT' AND '$(TargetFramework)' == 'net462'" />
	Condition="'$(TargetOs.ToUpper())' == 'WINDOWS_NT' AND '$(TargetFramework)' != 'net462'" />

[edwardneal]

Copilot is correct, although I'd go further: there's no need for conditional inclusion, this should always be included.

[benrr101]

@edwardneal you'll probably have to explain that a bit more. The way I'm looking at it, this file replaces UseManagedNetworking in the LocalAppContextSwitches with a constant if the consumer of the library specifies UseManagedNetworkingOnWindows as true or false in their project's csproj. But it does no check for whether the project is actually on windows or not. So if we include the file in all builds, we'll end up with these scenarios:

• netcore on Windows - behaves as expected, if user specifies the switch it'll replace with the constant appropriately
• netfx on Windows - allows user to replace UseManagedNetworking constant false with true which will break things
• netcore on Unix - allows user to replace UseManagedNetworking constant true with false which will break things

So as far as I can tell, we really do need to conditionally include the file to lead users into the "pit of success"

[edwardneal]

Specifying UseManagedNetworkingOnWindows in the csproj doesn't automatically rewrite the AppContext switch, or instruct the IL trimmer to trim away the unnecessary SNI. It just sets an MSBuild variable. In the future, once trimming is supportable, there'll be a custom .targets file within M.D.S which includes a <RuntimeHostConfigurationOption /> if that variable is set to a specific value - and it's RuntimeHostConfigurationOption which provides the instruction to the IL trimmer to perform the substitution you're describing.

With that in mind:

• netcore on Windows - completely valid.
• netfx on Windows - doesn't happen, netfx doesn't support RuntimeHostConfigurationOption or IL trimming.
• netcore on Unix - possible. It's the reason why I'd originally included an OS-specific ILLink.Substitutions.xml file.

At present, IL trimming isn't supported. At the point that it is, the custom .targets file can detect that a user is publishing to Unix with that configuration and raise a build error. We can also guard the check to use the managed SNI, such that a non-Windows OS will always reference the managed SNI irrespective of the value of the AppContext switch:

internal static class LocalAppContextSwitches
{
    // After IL trimming - this would normally check the AppContext switch.
    private bool UseManagedNetworkingOnWindows => true;

    public bool UseManagedNetworking =>
        !RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || UseManagedNetworkingOnWindows;
}

internal static class TdsParserStateObjectFactory
{
    internal TdsParserStateObject CreateSessionObject(...) =>
        return LocalAppContextSwitches.UseManagedNetworking
            ? new TdsParserStateObjectManaged(...)
            : new TdsParserStateObjectNative(...);
}

[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.

[edwardneal]

Copilot is correct, although I'd go further: there's no need for conditional inclusion, this should always be included.

[paulmedynski]

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

[paulmedynski]

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

[benrr101]

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

[paulmedynski]

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

[paulmedynski]

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

[benrr101]

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.

[paulmedynski]

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.

[benrr101]

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.

[paulmedynski]

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.

[benrr101]

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:

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? 😄

[paulmedynski]

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

[benrr101]

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? 😉

[paulmedynski]

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.

[paulmedynski]

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>

[paulmedynski]

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

[benrr101]

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?

[benrr101]

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

[paulmedynski]

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.

[mdaigle]

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

[mdaigle]

What does this line do?

[benrr101]

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.

[paulmedynski]

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 :)

[mdaigle]

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

[mdaigle]

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

[benrr101]

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 🤷‍♂️

[benrr101]

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

[mdaigle]

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.