Add | Adding Net6 support and dropping netcoreapp3.1 #1704

JRahnama · 2022-08-09T22:48:47Z

This PR is ready for review. The tests are passing on a parallel pipeline built for net6.

There are two major changes in this PR:

Support for netcoreapp3.1 is dropped.
Support for net6.0 is added.

I have left comments inside the changes wherever I was not sure, or change needed some explanation.

tools/props/Versions.props

tools/specs/Microsoft.Data.SqlClient.nuspec

.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj

src/Microsoft.Data.SqlClient/add-ons/Directory.Build.props

ErikEJ · 2022-09-25T09:06:18Z

Fingers crossed this gets in soon!

JRahnama · 2022-09-26T15:57:06Z

Fingers crossed this gets in soon!

I will do my best to add net6 to our pipelines to see if the tests are passing fine.

codecov · 2022-09-28T23:02:02Z

Codecov Report

Base: 71.49% // Head: 65.38% // Decreases project coverage by -6.10% ⚠️

Coverage data is based on head (cb9cbc2) compared to base (02daa6f).
Patch coverage: 71.42% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1704      +/-   ##
==========================================
- Coverage   71.49%   65.38%   -6.11%     
==========================================
  Files         290      290              
  Lines       61109    61236     +127     
==========================================
- Hits        43692    40042    -3650     
- Misses      17417    21194    +3777

Flag	Coverage Δ
addons	`0.00% <ø> (-92.39%)`	⬇️
netcore	`69.25% <71.42%> (-5.83%)`	⬇️
netfx	`63.65% <81.81%> (-5.67%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs	`62.42% <0.00%> (-21.44%)`	⬇️
...core/src/Microsoft/Data/SqlClient/SqlDataReader.cs	`71.36% <ø> (-1.90%)`	⬇️
...icrosoft/Data/SqlClient/SqlDelegatedTransaction.cs	`45.12% <ø> (-1.11%)`	⬇️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs	`72.76% <ø> (-0.13%)`	⬇️
...ore/src/Microsoft/Data/SqlClient/SqlTransaction.cs	`90.00% <ø> (ø)`
...c/Microsoft/Data/SqlTypes/SqlFileStream.Windows.cs	`53.61% <ø> (ø)`
...t/src/Microsoft/Data/Common/AdapterUtil.Windows.cs	`66.66% <ø> (ø)`
...SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs	`67.56% <ø> (-0.83%)`	⬇️
...viderBase/DbConnectionPoolAuthenticationContext.cs	`41.66% <ø> (ø)`
...ft/Data/Sql/SqlDataSourceEnumeratorNativeHelper.cs	`88.37% <ø> (ø)`
... and 74 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tools/props/Versions.props

cheenamalhotra · 2022-10-01T00:37:22Z

tools/specs/Microsoft.Data.SqlClient.nuspec

+        <dependency id="System.Text.Encoding.CodePages" version="6.0.0" exclude="Compile" />
+        <dependency id="System.Text.Encodings.Web" version="6.0.0" />
+        <dependency id="System.Resources.ResourceManager" version="4.3.0" />
+        <dependency id="System.Security.Cryptography.Cng" version="5.0.0" />


Please reflect dependency version updates in this file, for all groups.

tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec

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

JRahnama · 2022-10-20T05:54:30Z

.editorconfig

@@ -140,6 +140,8 @@ dotnet_diagnostic.CA1063.severity = silent
 # CA2100: Review SQL queries for security vulnerabilities
 dotnet_diagnostic.CA2100.severity = silent

+dotnet_diagnostic.CA1416.severity = silent


.NET 5.0 added new attributes, SupportedOSPlatformAttribute and UnsupportedOSPlatformAttribute, to annotate platform-specific APIs. Both attributes can be instantiated with or without version numbers as part of the platform name. They can also be applied multiple times with different platforms. However, SqlClient currently is following a different structure for adding related files for different platforms.

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs

JRahnama · 2022-10-20T06:00:03Z

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs

+#elif NETCOREAPP
+        public override void Rollback(string transactionName)
+#endif
+        { }


after net5.0 System.Data.Common.DbTransaction added Rollback(string) and Save(string). NetStandard does not support that change.

JRahnama · 2022-10-20T06:01:10Z

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

-  </PropertyGroup>
-  <PropertyGroup Condition="'$(TargetGroup)' == 'netstandard'">
-    <DefineConstants>$(DefineConstants);NETSTANDARD;</DefineConstants>
-  </PropertyGroup>


using bult-in constants instead of creating new ones.

JRahnama · 2022-10-20T06:03:44Z

.../netcore/src/Microsoft/Data/SqlClient/SqlColumnEncryptionCertificateStoreProvider.Windows.cs

+#else
+            keySizeInBytes= RSAPublicKey.KeySize / 8;
+#endif
+


PublicKey.Key is obsolete. Currently GetRSAPublicKey is being used but using DSAKey might be added in future.

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

JRahnama · 2022-10-20T06:07:32Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/ParametersTest.cs

@@ -223,7 +223,7 @@ public static void Test_WithDecimalValue_ShouldReturnDecimal()
            var cmd = new SqlCommand("select @foo", conn);
            cmd.Parameters.AddWithValue("@foo", new SqlDecimal(0.5));
            var result = (decimal)cmd.ExecuteScalar();
-            Assert.Equal(result, (decimal)0.5);
+            Assert.Equal((decimal)0.5, result);


upgrading xunit to 2.4.2 discovered these mistakes. expected values needs to be the first and actual value has to come second.

JRahnama · 2022-10-20T06:09:08Z

...sts/tools/Microsoft.Data.SqlClient.ExtUtilities/Microsoft.Data.SqlClient.ExtUtilities.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
-    <TargetFrameworks>netcoreapp3.1;net5.0</TargetFrameworks>
+    <TargetFrameworks>net6.0</TargetFrameworks>


this could be changed to <TargetFramework> and it will eliminate the need to specifying framework on build time on test pipelines.

using TargetFramework would mean that we don't build this for netfx, is that correct? and if so is it not being needed for netfx intentional?

Correct, This is for tests builds on Unix,. Currently when you try to build the database for the test suit, Northwind, you need to run this project by specifying the dotnet run -f net6.0, by doing so the need to specifying the TFM in dotnet command goes away.

JRahnama · 2022-10-20T06:09:53Z

...lClient/tests/tools/Microsoft.DotNet.XUnitExtensions/Microsoft.DotNet.XUnitExtensions.csproj

@@ -48,7 +48,7 @@
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(MicrosoftNETTestSdkVersion)" />
    <PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="$(SystemRuntimeInteropServicesRuntimeInformationVersion)" />
    <PackageReference Include="xunit" Version="$(XunitVersion)" />
-    <PackageReference Include="xunit.runner.visualstudio" Version="$(XunitVersion)" />
+    <PackageReference Include="xunit.runner.visualstudio" Version="$(xunitrunnervisualstudioVersion)" />


xunit.runner.visualstudio now has a different version than xunit itself.

JRahnama · 2022-10-20T06:15:26Z

tools/targets/NotSupported.targets

@@ -3,7 +3,7 @@
  <PropertyGroup Condition="'$(GeneratePlatformNotSupportedAssembly)' == 'true' OR '$(GeneratePlatformNotSupportedAssemblyMessage)' != ''">
    <!-- Tell ResolveMatchingContract to run and resolve contract to project reference -->
    <ResolveMatchingContract>true</ResolveMatchingContract>
-    <NotSupportedSourceFile>$(IntermediateOutputPath)$(AssemblyName).notsupported.cs</NotSupportedSourceFile>
+    <NotSupportedSourceFile>$(IntermediateOutputPath)\$(TargetFramework)\$(AssemblyName).notsupported.cs</NotSupportedSourceFile>


Since System.Data.Common.DbTransaction is changed for net5 by adding RollBack(string) and Save(string) we cannot have one file for netcore and netstandard. It will complain that there is no suitable method to be overridden for netstandard. Taking the notsupported file inside each framework solves that issue. I would appreciate all other ideas regarding the impact of this change.
@cheenamalhotra @David-Engel

One notsupported.cs file for each build target is the safe way to do this.

I may not be understanding the problem, but in Npgsql this is just a tiny #ifdef:

#if NET5_0_OR_GREATER public override void Save(string name) #else public void Save(string name) #endif {

@roji I noticed I did not provide more input on the question. As you have mentioned we have done the same in the code. The problem/question starts when generating notsupported files using GenAPI. Previously there was one Microsoft.Data.SqlClient.notSupported file for netcore and netstandards and one for netfx. By this change GenAPI complains that there is no suitable method to override for these 2 methods. I made the pattern so each TFM gets a not supported file and I was wondering if that will cause some issues in future.

ErikEJ · 2022-10-20T07:53:00Z

LGTM

ErikEJ · 2022-10-20T08:33:37Z

228 changed files??

Wraith2 · 2022-10-20T11:01:05Z

Looks good to me in general. It's a complex set of changes so I'm sure we'll find runtime things that need to be fixed once we've got this merged but i think we just need to get on and track those down as we find them.

JRahnama · 2022-10-20T15:27:40Z

228 changed files??

that was a mistake I did while addressing some conflicts. The PR has been opened for a very long time and there were several conflicts. I had to force push a new clone to fix that issue. it is 38 files.

ErikEJ · 2022-10-20T15:31:11Z

@JRahnama yeah. I realised. Looking forward to getting this in so work on DateOnly and TimeOnly can get done before EF Core 8.

Wraith2 · 2022-10-20T16:14:09Z

@JRahnama yeah. I realised. Looking forward to getting this in so work on DateOnly and TimeOnly can get done before EF Core 8.

If you want to do Date and Time i'll work on SqlBatch

ErikEJ · 2022-10-20T16:44:03Z

@Wraith2 Sure, willing to give it a try.

…mprovments.

Wraith2 · 2022-10-22T10:04:59Z

Yay!

ErikEJ · 2022-10-22T10:11:07Z

Amazing!

Wraith2 · 2022-10-24T19:34:49Z

I think a reference got missed somewhere around the functional tests. If I try to build locally I get this:

C:\Program Files\Microsoft Visual Studio\2022\Professional\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(1805,5): error : Project 'E:\Programming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\add-ons\AzureKeyVaultProv
ider\Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj' targets 'net6.0'. It cannot be referenced by a project that targets '.NETCoreApp,Version=v3.1'. [E:\Programming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.Tests.csproj]

build script i use is buildandrun.bat from above root.

@echo off

set netcore=net6.0
set config=Release

msbuild /t:clean
msbuild /p:Configuration=%config%
msbuild /t:BuildTestsNetCore /p:TargetNetCoreVersion=%netcore% /p:Configuration=%config%

call :pauseOnError  dotnet test "src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.Tests.csproj" /p:Platform="AnyCPU" /p:Configuration="%config%" /p:TestTargetOS="Windowsnetcoreapp" /p:TargetNetCoreVersion=%netcore%  --no-build -v n --filter "category!=nonnetcoreapptests&category!=failing&category!=nonwindowstests"

goto :eof

:pauseOnError
%*
if ERRORLEVEL 1 pause
goto :eof

JRahnama · 2022-10-24T21:23:38Z

I think a reference got missed somewhere around the functional tests. If I try to build locally I get this:

C:\Program Files\Microsoft Visual Studio\2022\Professional\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(1805,5): error : Project 'E:\Programming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\add-ons\AzureKeyVaultProv
ider\Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj' targets 'net6.0'. It cannot be referenced by a project that targets '.NETCoreApp,Version=v3.1'. [E:\Programming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.Tests.csproj]

build script i use is buildandrun.bat from above root.

@echo off

set netcore=net6.0
set config=Release

msbuild /t:clean
msbuild /p:Configuration=%config%
msbuild /t:BuildTestsNetCore /p:TargetNetCoreVersion=%netcore% /p:Configuration=%config%

call :pauseOnError  dotnet test "src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.Tests.csproj" /p:Platform="AnyCPU" /p:Configuration="%config%" /p:TestTargetOS="Windowsnetcoreapp" /p:TargetNetCoreVersion=%netcore%  --no-build -v n --filter "category!=nonnetcoreapptests&category!=failing&category!=nonwindowstests"

goto :eof

:pauseOnError
%*
if ERRORLEVEL 1 pause
goto :eof

Interesting. Can you try building the test with -p:TF=net6.0 or as you have been doing /p:TF=net6.0 instead of TargetNetCoreVersion to see if that solves the issue?

Wraith2 · 2022-10-24T23:12:25Z

Using TF works for netfx. for net6.0 it seems to mostly work but then can't find the output functional tests.

Test run for E:\Programming\csharp7\SqlClient\artifacts\Project\bin\Windows_NT\Debug.AnyCpu.FunctionalTests\net6.0\FunctionalTests.dll (.NETCoreApp,Version=v6.0)
Microsoft (R) Test Execution Command Line Tool Version 17.4.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

The test source file "E:\Programming\csharp7\SqlClient\artifacts\Project\bin\Windows_NT\Debug.AnyCpu.FunctionalTests\net6.0\FunctionalTests.dll" provided was not found.
     1>Project "E:\Programming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlC
       lient.Tests.csproj" on node 1 (VSTest target(s)).
     1>DispatchToInnerBuildsWithVSTestTarget:
         Build continuing because "ContinueOnError" on the task "MSBuild" is set to "ErrorAndContinue".
     1>Done Building Project "E:\Programming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\tests\FunctionalTests\Micro
       soft.Data.SqlClient.Tests.csproj" (VSTest target(s)) -- FAILED.

JRahnama · 2022-10-25T03:17:10Z

@Wraith2 I was not able to reproduce the issue you mentioned, and pipelines are working well. I saw that error while I was working on the pipeline. It was because TargetGroup was not defined properly. Can you make a fresh clone and test the scenario again please?

ErikEJ reviewed Aug 10, 2022

View reviewed changes

tools/props/Versions.props Outdated Show resolved Hide resolved

ErikEJ reviewed Aug 10, 2022

View reviewed changes

tools/props/Versions.props Outdated Show resolved Hide resolved

ErikEJ reviewed Aug 10, 2022

View reviewed changes

tools/specs/Microsoft.Data.SqlClient.nuspec Outdated Show resolved Hide resolved

JRahnama added 📃 Documentation Use this label for any documentation changes and removed 📃 Documentation Use this label for any documentation changes labels Aug 10, 2022

JRahnama added this to To Do in SqlClient v5.1 Aug 10, 2022

JRahnama modified the milestones: 5.1.0, 5.1.0-preview1 Aug 10, 2022

dotMorten reviewed Sep 6, 2022

View reviewed changes

.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/add-ons/Directory.Build.props Show resolved Hide resolved

JRahnama mentioned this pull request Sep 6, 2022

Issue with error CS0114 and CS0109 dotnet/roslyn#63821

Closed

JRahnama marked this pull request as ready for review September 6, 2022 23:40

JRahnama changed the title ~~Draft | Adding Net6 support to the driver~~ Add | Adding Net6 support to the driver Sep 6, 2022

JRahnama mentioned this pull request Sep 7, 2022

Fix potential security vulnerabilities. #1612

Closed

cheenamalhotra reviewed Oct 1, 2022

View reviewed changes

JRahnama modified the milestones: 5.1.0-preview1, 5.1.0-preview2 Oct 5, 2022

lcheunglci reviewed Oct 18, 2022

View reviewed changes

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

JRahnama commented Oct 20, 2022

View reviewed changes

JRahnama requested review from ErikEJ, Wraith2, cheenamalhotra and lcheunglci and removed request for ErikEJ and Wraith2 October 20, 2022 06:17

JRahnama requested review from ErikEJ and Wraith2 and removed request for ErikEJ October 20, 2022 06:18

commit

16814eb

JRahnama force-pushed the net6-build branch from 05db228 to 16814eb Compare October 20, 2022 10:37

Wraith2 approved these changes Oct 20, 2022

View reviewed changes

ErikEJ approved these changes Oct 20, 2022

View reviewed changes

version update

6801c48

Removing Secure password which is obsolete and addressing some test i…

49a0042

…mprovments.

JRahnama changed the title ~~Add | Adding Net6 support to the driver~~ Add | Adding Net6 support and dropping netcoreapp3.1 Oct 20, 2022

Addressing binary formatter

cb9cbc2

lcheunglci approved these changes Oct 20, 2022

View reviewed changes

David-Engel approved these changes Oct 21, 2022

View reviewed changes

SqlClient v5.1 automation moved this from To Do to Review Approved Oct 21, 2022

JRahnama merged commit 20d4c19 into dotnet:main Oct 22, 2022

SqlClient v5.1 automation moved this from Review Approved to Done Oct 22, 2022

JRahnama deleted the net6-build branch October 31, 2022 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add | Adding Net6 support and dropping netcoreapp3.1 #1704

Add | Adding Net6 support and dropping netcoreapp3.1 #1704

JRahnama commented Aug 9, 2022 •

edited

ErikEJ commented Sep 25, 2022

JRahnama commented Sep 26, 2022

codecov bot commented Sep 28, 2022 •

edited

cheenamalhotra Oct 1, 2022

JRahnama Oct 20, 2022

JRahnama Oct 20, 2022

JRahnama Oct 20, 2022

JRahnama Oct 20, 2022

JRahnama Oct 20, 2022

JRahnama Oct 20, 2022

Wraith2 Oct 20, 2022

JRahnama Oct 20, 2022

JRahnama Oct 20, 2022

JRahnama Oct 20, 2022

Wraith2 Oct 20, 2022

roji Oct 20, 2022

JRahnama Oct 20, 2022

ErikEJ commented Oct 20, 2022

ErikEJ commented Oct 20, 2022

Wraith2 commented Oct 20, 2022

JRahnama commented Oct 20, 2022

ErikEJ commented Oct 20, 2022

Wraith2 commented Oct 20, 2022

ErikEJ commented Oct 20, 2022

Wraith2 commented Oct 22, 2022

ErikEJ commented Oct 22, 2022

Wraith2 commented Oct 24, 2022 •

edited

JRahnama commented Oct 24, 2022 •

edited

Wraith2 commented Oct 24, 2022

JRahnama commented Oct 25, 2022

Add | Adding Net6 support and dropping netcoreapp3.1 #1704

Add | Adding Net6 support and dropping netcoreapp3.1 #1704

Conversation

JRahnama commented Aug 9, 2022 • edited

ErikEJ commented Sep 25, 2022

JRahnama commented Sep 26, 2022

codecov bot commented Sep 28, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikEJ commented Oct 20, 2022

ErikEJ commented Oct 20, 2022

Wraith2 commented Oct 20, 2022

JRahnama commented Oct 20, 2022

ErikEJ commented Oct 20, 2022

Wraith2 commented Oct 20, 2022

ErikEJ commented Oct 20, 2022

Wraith2 commented Oct 22, 2022

ErikEJ commented Oct 22, 2022

Wraith2 commented Oct 24, 2022 • edited

JRahnama commented Oct 24, 2022 • edited

Wraith2 commented Oct 24, 2022

JRahnama commented Oct 25, 2022

JRahnama commented Aug 9, 2022 •

edited

codecov bot commented Sep 28, 2022 •

edited

Wraith2 commented Oct 24, 2022 •

edited

JRahnama commented Oct 24, 2022 •

edited