Improve SNI package architecture and targets file design #2998
Comments
|
I have tried directly referencing Microsoft.Data.SqlClient and separately Microsoft.Data.SqlClient.SNI and neither one gets copied. CopySNIFiles doesn't get run when running under a MSA Group account. I don't understand why, although I also really doubt i would have written the SNI.targets file this way. |
|
The issue seems to be that, when running on the build server, OutDir has a different value, which includes the TargetFramework. We have a csproj with: <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net48</TargetFramework>
<AppendTargetFrameworkToOutputPath>false<AppendTargetFrameworkToOutputPath>
<AppendRuntimeIdentifierToOutputPath>false</AppendRuntimeIdentifierToOutputPath>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Data.SqlClient" Version="5.2.2" />
</ItemGroup>
</Project>This results in OutDir evaluating to bin\Release\net48 but ONLY due to SNI targets getting evaluated at restore time, which apparently somehow ignores the above AppendX properties. |
|
@ErikEJ Why doesn't Microsoft.Data.SqlClient.SNI package things the way Microsoft recommends as best practice? https://learn.microsoft.com/en-us/nuget/create-packages/supporting-multiple-target-frameworks and https://learn.microsoft.com/en-us/nuget/create-packages/native-files-in-net-packages#native-assets In comparing what your team is doing to what Grpc.Core does , they seem to be exactly following the best practices, and I don't run into any issues using their library. I actually can't find where Microsoft.Data.SqlClient.SNI.targets is defined in this repository, so I can't even submit a PR to rewrite the thing to follow best practices. |
|
I tested your csproj in a standalone project and I don't face any issues, and SNI files are being copied in output folder alongside other binaries as expected. You must note the 'buildTransitive' folder is where the files will be copied from in this case.
[Please open the child Task in your screenshot, you should find this IMO] Using
Why is this the case in your build server? Are you modifying it manually? |
cheenamalhotra
added
the
✔️ Triage Done
|
The project runs fine, on the build server, with me as the user. When run as a MSA Group Principal (passwordless login managed by Active Directory), it does not copy the files for some reason. I want to emphasize, I do NOT run into this problem with Grpc.Core, and i do NOT see issues in the Grpc.Core library related to native dlls not loading correctly. This repository has more issues than I can count with loading SNI.dll. I think it is time to rethink the approach. |
|
See also dotnet/msbuild#10973 |
|
Thanks to the reference to Grpc.Core and I like that it's neat. Our design of MDS.SNI targets file is based on legacy systems - I agree. If's been challenging to keep up with MsBuild targets as the design we have interjects CopySNIFiles target into MsBuild process, but is clearly not 100% proof. We will review if we can adopt a much cleaner approach without risking existing consumption. |
cheenamalhotra
added
the
💡 Enhancement
cheenamalhotra
changed the title
cheenamalhotra
added
the
Area\Native SNI
|
To support all systems, I think there are a couple of basic scenarios. I don't believe SNI supports Linux or Mac OSX, so there would be no folders for those. The only issue I am not confident that would be resolved by the rewrite is the IIS shadow copy issue. I personally don't actually understand that issue, although I may have to deal with it next but doubt it as I use Octopus Deploy for IIS deployments and they have written robust logic to stop App Pools that Visual Studio right-click deploy does not do. I am currently trying to figure out a workaround that will allow the MSA Group Principal to copy these files. Otherwise, I have to revert 120+ files related to moving from System.Data.SqlClient and stay on that for as long as it takes to resolve this issue. That is not ideal as EntityFramework 6.5.1 seems incompatible with System.Data.SqlClient (lazy loading is now broken, and @ErikEJ suggested I upgrade to Microsoft.Data.SqlClient). |
|
You can include these DLLs directly in your csproj as the targets file in Grpc.Core is doing fyi, have you tried with that? |
|
I basically tried that already. The following did not work running as a MSA Group Principal but did work locally. <Project Sdk="Microsoft.NET.Sdk">
<!-- ReSharper disable UnknownProperty -->
<PropertyGroup>
<AssemblyTitle>Weiss.Panda.TaskRunner</AssemblyTitle>
<TargetFramework>net48</TargetFramework>
<OutputType>Exe</OutputType>
<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
<AppendRuntimeIdentifierToOutputPath>false</AppendRuntimeIdentifierToOutputPath>
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
<GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType>
<CopySNIFiles>false</CopySNIFiles> <!-- THIS IS WHERE I DISABLE the SNI.targets BEHAVIOR -->
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Data.SqlClient.SNI" Version="5.2.0" GeneratePathProperty="true" />
</ItemGroup>
<Target Name="SNITest" AfterTargets="CopySNIFiles">
<Message Text="Files: @(SNIFiles)" />
<Message Text="DLL Files: @(SNIDllFiles)" />
</Target>
<ItemGroup>
<None Include="$(PkgMicrosoft_Data_SqlClient_SNI)\buildTransitive\net462\*"
Exclude="$(PkgMicrosoft_Data_SqlClient_SNI)\buildTransitive\net462\*.targets"
CopyToOutputDirectory="PreserveNewest"
Pack="true"
PackagePath="" />
</ItemGroup>
</Project>This produces the following trace in binlog:
If I also search for the phrase, |
|
I still don't understand what exactly are you doing with the MSA Group Principal (I'm not familiar with what its used for), is it a build or publish step that is not working? |
|
The problem occurs on build. An MSA Group Principal is just a special account that has no password. You simply tell your Active Directory Domain Controller that a specific Machine SID is associated with a particular account, and that account is a principal on that Machine SID. Once they become linked, you no longer need a password to login. From a cybersecurity policy perspective, this is preferred to running as NT AUTHORITY\System as it can have less trust. We've actually been running Team City this way for about 5 years, and this is the first time I have encountered this weird issue. To say I am curious as to what the root cause is, is an understatement. About the only conclusion I can draw from this full day of debugging is that Microsoft doesn't use MSA Group Principals to build its source code. :) |
|
No, we don't as we use DevOps workflows designed to spin up environment on demand. It seems more like an MsBuild issue to me when working with MSA accounts than ours. I hope you get the right support from them. |
|
I can confirm that adding a PowerShell script to copy the files to some random directory on the same drive It seems as though there is more to this problem than just MSA accounts. Will repeat my offer to help write .targets for this $dest = "d:\logs\TeamCity"
$source = Join-Path $env:USERPROFILE -ChildPath ".nuget" | Join-Path -ChildPath "packages" | Join-Path -ChildPath "microsoft.data.sqlclient.sni" | Join-Path -ChildPath "5.2.0"
Write-Host $source
if (Test-Path $source) {
# Copy all files from the source directory to the destination directory
$copyErrors = $null
Copy-Item -Path "$source\*" -Destination $dest -Recurse -Force -Verbose -ErrorVariable +copyErrors
Write-Host "Files have been copied from '$source' to '$dest'."
Write-Host "Errors: $copyErrors"
}Outputs: |
MDS does package the recommended way when targeting .NET. The problem is you are targeting .NET Framework, which doesn't support all the new, simple packaging stuff. https://learn.microsoft.com/en-us/nuget/create-packages/native-files-in-net-packages#projects-targeting-net-framework If you want to try improving the targets file, it's in the NuGet package: https://www.nuget.org/packages/Microsoft.Data.SqlClient.SNI/ Open the nupkg file with 7-Zip (or rename the file to .zip) and look in the buildTransitive\net462 folder. All you need to rebuild the package is a complete nuspec file. You can use this: Just unzip the nupkg, adjust any paths in the nuspec file (I think it will work for you as-is) and run nuget pack Microsoft.Data.SqlClient.SNI.nuspec. |
|
Does setting the MSBuild property |
|
@HamsterExAstris Basically yes see #2998 (comment) which is a variation... and you can see, in the msbuild binlog, that it picks up the file and says it is copied. I should figure this out next week. I'm going to explore the possibility there is a bug in msbuild Copy target next. I believe the number of cores my desktop has is different than the AWS m6.2xlarge we are using as a build server, so it's possible the bug is appearing when a certain number of threads copy data. I really doubt it but I have to rule that out next. It also doesn't explain why running as local administrator on the m6.2xlarge works, but maybe there is a side effect introduced by PS Remoting that implicitly limits threads somehow or something else. |
|
I'm going to try this
with this #2998 (comment) This will take MSBuild Copy target out of the picture and replace it with a <Target Name="AfterBuild">
<PropertyGroup>
<PSScript> paste ps script here
</PSScript>
</PropertyGroup>
<Exec Command="powershell.exe -command "$(PSScript)"" />
</Target>This allows me to test a linear array of assumptions about the root cause. |
|
As a follow-up here, I believe the root cause here is that the way Microsoft.Data.SqlClient.SNI is packaged is incompatible with a clean checkout on a build due to how This is very likely related to #2049 (comment) and possibly other issues users have been pulling their hair out over. |
|
@David-Engel @cheenamalhotra Would it be possible to add a |
The easiest way to play with changes to the .targets file is to simply edit the one in your NuGet cache. Example: Once you have something that fixes your situation, share the targets file here for others to test. |
|
Did some reading tonight to see if there is an elegant path forward. One possible thing I will try this week is explicit nuspec assembky I'm also going to thoroughly understand why Grpc.Core doesn't give me these issues. I do appreciate all your help, David and Cheena. Hopefully whatever I come up with makes your team job easier. However, not having the packaging in version control makes it harder to understand each incremental change as there is no audit trail on the issues that effectuate changes. |
There have only been 8 changes to the history of the file. Not sure if it will help, but I've pushed those commits here for reference: https://github.com/David-Engel/SqlClient/commits/sni-targets/sni-ref/Microsoft.Data.SqlClient.SNI.targets |
|
Is it acceptable use to put a sample package on nuget.org? Package name would be something like "JohnZabroskiTest.Microsoft.Data.SqlClient.SNI" and would have a alpha pre-release tag on it as well. I can also de-list the package to avoid confusion. However, it would contain binaries shipped by Microsoft and/or .NET Foundation. I may be able to host one here on GitHub as well but never done it before and I believe the only way to push a package up to GitHub packages repository is thru a GitHub action. However, putting it on nuget.org would be the lowest friction approach. |
For testing, you can also just update your local or project's NuGet.config and add a local repository like: |
I am pulling my hair out trying to figure out why SNI.x64.dll isn't getting copied to my output directory on build.
i believe the issue has something to do with x86 vs x64 wow process virtualization, but I don't know how to prove it. Our TeamCity account runs as a MSA Group Principal DOMAINNAME\msaTeamCity$ and that users package cache is
C:\Users\msaTeamCity$\.nuget\packages\microsoft.data.sqlclient.sni\5.2.0\When I use msbuild binlogging, I see CopySNIFiles silently fails
I think this is probably the root cause of a litter of issues people have, although I suspect there is even more that can be cleaned up here. Such discussion is probably best saved for a WhatsApp or Discord voice call.
The text was updated successfully, but these errors were encountered: