Delete System.Data.SqlClient package #2275

jkotas · 2020-01-28T08:12:08Z

This package is superseded by Microsoft.Data.SqlClient that lives in https://github.com/dotnet/SqlClient repo.

Fixes https://github.com/dotnet/corefx/issues/40846

src/libraries/shims/ApiCompatBaseline.netcoreapp.netstandard.txt

src/coreclr/tests/scripts/run-corefx-tests.bat

stephentoub · 2020-01-28T13:45:09Z

What about the references in

runtime/src/libraries/pkg/descriptions.json

Lines 550 to 568 in b8942c3

    
             "Name": "System.Data.SqlClient", 
        
             "Description": "Provides the data provider for SQL Server. These classes provide access to versions of SQL Server and encapsulate database-specific protocols, including tabular data stream (TDS)", 
        
             "CommonTypes": [ 
        
               "System.Data.SqlClient.SqlConnection", 
        
               "System.Data.SqlClient.SqlException", 
        
               "System.Data.SqlClient.SqlParameter", 
        
               "System.Data.SqlDbType", 
        
               "System.Data.SqlClient.SqlDataReader", 
        
               "System.Data.SqlClient.SqlCommand", 
        
               "System.Data.SqlClient.SqlTransaction", 
        
               "System.Data.SqlClient.SqlParameterCollection", 
        
               "System.Data.SqlClient.SqlClientFactory" 
        
             ] 
        
           }, 
        
           { 
        
             "Name": "System.Data.SqlClient.sni", 
        
             "Description": "Internal implementation package not meant for direct consumption.  Please do not reference directly.  Provides native implementation dll used by System.Data.SqlClient.", 
        
             "CommonTypes": [] 
        
           },

runtime/src/libraries/pkg/Microsoft.Private.PackageBaseline/packageIndex.json

Lines 1416 to 1430 in b8942c3

    
           "System.Data.SqlClient": { 
        
             "StableVersions": [ 
        
               "4.1.0", 
        
               "4.3.0", 
        
               "4.3.1", 
        
               "4.4.0", 
        
               "4.4.1", 
        
               "4.4.2", 
        
               "4.4.3", 
        
               "4.5.0", 
        
               "4.5.1", 
        
               "4.6.0", 
        
               "4.6.1", 
        
               "4.7.0" 
        
             ],

runtime/src/mono/netcore/CoreFX.issues_windows.rsp

Line 30 in b8942c3

-nonamespace System.Data.SqlClient.Tests

runtime/src/mono/mono/metadata/assembly.c

Line 163 in b8942c3

FACADE_ASSEMBLY ("System.Data.SqlClient"),

runtime/.config/CredScanSuppressions.json

Lines 20 to 25 in b8942c3

    
           "/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsEnums.cs", 
        
           "/src/libraries/System.Data.SqlClient/tests/FunctionalTests/DiagnosticTest.cs", 
        
           "/src/libraries/System.Data.SqlClient/tests/FunctionalTests/ExceptionTest.cs", 
        
           "/src/libraries/System.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs", 
        
           "/src/libraries/System.Data.SqlClient/tests/Tools/TDS/TDS.Servers/TDSServerArguments.cs", 
        
           "/src/libraries/System.Data.SqlClient/tests/Tools/TDS/TDS.Servers/TdsServerCertificate.pfx",

etc.?

jkotas · 2020-01-28T14:16:19Z

runtime/src/libraries/pkg/descriptions.json

Lines 550 to 568 in b8942c3

"Name": "System.Data.SqlClient",

"Description": "Provides the data provider for SQL Server. These classes provide access to versions of SQL Server and encapsulate database-specific protocols, including tabular data stream (TDS)",

"CommonTypes": [

"System.Data.SqlClient.SqlConnection",

"System.Data.SqlClient.SqlException",

"System.Data.SqlClient.SqlParameter",

"System.Data.SqlDbType",

"System.Data.SqlClient.SqlDataReader",

"System.Data.SqlClient.SqlCommand",

"System.Data.SqlClient.SqlTransaction",

"System.Data.SqlClient.SqlParameterCollection",

"System.Data.SqlClient.SqlClientFactory"

]

},

{

"Name": "System.Data.SqlClient.sni",

"Description": "Internal implementation package not meant for direct consumption. Please do not reference directly. Provides native implementation dll used by System.Data.SqlClient.",

"CommonTypes": []

},

runtime/src/libraries/pkg/Microsoft.Private.PackageBaseline/packageIndex.json

Lines 1416 to 1430 in b8942c3

"System.Data.SqlClient": {

"StableVersions": [

"4.1.0",

"4.3.0",

"4.3.1",

"4.4.0",

"4.4.1",

"4.4.2",

"4.4.3",

"4.5.0",

"4.5.1",

"4.6.0",

"4.6.1",

"4.7.0"

],

These lists have references to all packages that were ever built in CoreFX, even many dead ones. I have kept it that way for consistency. @ericstj Let me know if SqlClient should be cleaned up from these lists.

runtime/src/mono/netcore/CoreFX.issues_windows.rsp

Line 30 in b8942c3

-nonamespace System.Data.SqlClient.Tests

Fixed.

runtime/src/mono/mono/metadata/assembly.c

Line 163 in b8942c3

FACADE_ASSEMBLY ("System.Data.SqlClient"),

This list is for legacy Mono. Nothing is changing for legacy Mono with this change. These sources live in dotnet/runtime as side-effect of bulk source sharing with legacy Mono.

runtime/.config/CredScanSuppressions.json

Lines 20 to 25 in b8942c3

"/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsEnums.cs",

"/src/libraries/System.Data.SqlClient/tests/FunctionalTests/DiagnosticTest.cs",

"/src/libraries/System.Data.SqlClient/tests/FunctionalTests/ExceptionTest.cs",

"/src/libraries/System.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs",

"/src/libraries/System.Data.SqlClient/tests/Tools/TDS/TDS.Servers/TDSServerArguments.cs",

"/src/libraries/System.Data.SqlClient/tests/Tools/TDS/TDS.Servers/TdsServerCertificate.pfx",

Fixed.

ericstj · 2020-01-28T16:58:52Z

packageIndex.json

The PackageIndex.json shouldn't be changed. We need this in order to emit the right package dependencies for libraries which depend on SqlClient. It looks like OleDb still depends on SqlClient.

runtime/src/libraries/System.Data.OleDb/src/System.Data.OleDb.csproj

Line 105 in 3b9abae

If that lib does indeed need SqlClient, then we should also restore the old package to replace the live-built ref.

src/libraries/restore/binplacePackages/binplacePackages.depproj

Wraith2 · 2020-01-28T21:46:04Z

If that lib does indeed need SqlClient, then we should also restore the old package to replace the live-built ref.

It's referenced but only one file imports the namespace and nothing from it is used. It's trivial to remove the dependency.

ericstj · 2020-01-28T22:40:28Z

Much better to remove it completely if it isn’t needed.

We will still need something like the restore step to get the System.Data shim correct.

jkotas · 2020-01-28T23:16:19Z

Much better to remove it completely if it isn’t needed.
We will still need something like the restore step to get the System.Data shim correct.

I have done both these. Thanks!

The last problem that I have is how to drop System.Data.SqlClient.dll into the testhost. System.Data.Common tests depend on it. What is the best way to drop System.Data.SqlClient.dll into the testhost?

Wraith2 · 2020-01-30T00:04:31Z

All the code references in the tests seem to be from DbProviderFactoriesTests and could be replaced with another provider as far as I can see.

ViktorHofer · 2020-02-03T19:41:43Z

Doesn't binplacing System.Data.SqlClient from a package into the runtime/testhost directory mean that during sourcebuild we need that package as a pre-built?

Wraith2 · 2020-02-03T20:22:47Z

If you only need the sqlclient library for the System.Data tests you could just replace the provider tests with OleDB or ODBC as the provider and then you don't need sqlclient or it's tests for this build do you?

jkotas · 2020-02-03T21:25:55Z

I need System.Data.SqlClient references for the source build, and I would like to keep it for running tests as well (e.g. the new test that I am adding in this PR). I do not think it should be a problem for source build:

References are handled via ilasm from .il for source build
Tests are not a problem for source build.

src/libraries/shims/manual/System.Data.csproj

ericstj · 2020-02-04T19:41:51Z

Doesn't binplacing System.Data.SqlClient from a package into the runtime/testhost directory mean that during sourcebuild we need that package as a pre-built?

Depends on what part of the build needs it. Today, we already have a prebuilt requirement for shims which is the only part of the product build that requires this. We do a two-pass build, where first pass can consume the reference, second pass can use the generated source. I think we can piggy-back on that.

Tests are already are allowed to have prebuilts in source build.

safern · 2020-02-05T08:44:14Z

The last problem that I have is how to drop System.Data.SqlClient.dll into the testhost.

We already restore test projects, why not just add a PackageReference in the test project to the given package?

jkotas · 2020-02-05T13:31:43Z

why not just add a PackageReference in the test project to the given package?

This is what I have done - the last two commits are the change required to flip from including it in the runtime to making the PackageReference work). It is hitting a bug in Mono assembly loader. I have disabled the affected test and will file an issue on it if the rest looks good.

This is passing the CI now. The change should be ready to review!

src/libraries/shims/manual/System.Data.csproj

....Runtime.Serialization.Formatters/tests/System.Runtime.Serialization.Formatters.Tests.csproj

ViktorHofer

Thanks, LGTM. I would like to know why we need to copy into the runtime/testhost before merging though.

Fixes https://github.com/dotnet/corefx/issues/40846

- Add System.Data.SqlClient package references for tests that need it - Use System.Data.SqlClient reference assembly to build the System.Data shim - Disable ILLinker for the shims since it does not work well with incremental builds and it is not necessary anyway Try to use package references in tests

jkotas · 2020-02-05T17:33:28Z

why we need to copy into the runtime/testhost before merging though.

We do not copy it in my latest version of the change. I have switched the tests that need it to have local package references and removed copying of System.Data.SqlClient.dll into runtime/testhost.

For reference, here is the list of places where System.Data.SqlClient.dll is placed under artifacts\bin after running build.cmd. All of them are refs.

\runtime\artifacts\bin\binplacePackages\netcoreapp5.0-Release\System.Data.SqlClient.dll
\runtime\artifacts\bin\binplacePackages\netstandard2.0-Release\System.Data.SqlClient.dll
\runtime\artifacts\bin\netstandard\netstandard2.0-Release\System.Data.SqlClient.dll
\runtime\artifacts\bin\ref\netcoreapp5.0\System.Data.SqlClient.dll
\runtime\artifacts\bin\ref\netstandard2.0\System.Data.SqlClient.dll

ViktorHofer · 2020-02-05T18:11:32Z

Thanks, I thought I still saw the PackageReference in runtime.depproj but maybe that was an old commit. Thanks for not writing into the runtime/testhost folder 👍 +💯

jkotas · 2020-02-05T18:50:29Z

The test failures are known issues:

Failed to acquire dotnet install script
Docker container recycling problem

safern · 2020-02-06T03:55:24Z

@jkotas it seems like this broke the rolling build tests on Linux_musl_arm64 (Alpine).

Binary formatter tests are failing: https://helix.dot.net/api/2019-06-17/jobs/ed130407-35ad-40cc-a9ef-24b9767b2a77/workitems/System.Runtime.Serialization.Formatters.Tests/console

The Binary Data that is failing seems to be System.Data objects. L581 of https://github.com/dotnet/runtime/blob/master/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs#L581 (GH won't show it since it is too big).

Also, System.Data.Common: https://helix.dot.net/api/2019-06-17/jobs/cb568d05-e0c9-4b5d-a292-1ac6dd5847a9/workitems/System.Data.Common.Tests/console

https://dev.azure.com/dnceng/public/_build/results?buildId=509595&view=ms.vss-test-web.build-test-results-tab

It could be related to the asset in the package reference having a bug or not picking the right one for Linux_musl_arm64 but not sure.

jkotas · 2020-02-06T11:40:56Z

It could be related to the asset in the package reference having a bug or not picking the right one for Linux_musl_arm64 but not sure.

Right, the problem is that the build for musl picks up lib\netcoreapp2.1\System.Data.SqlClient.dll from the https://www.nuget.org/packages/System.Data.SqlClient/ . This .dll is a stub implementation that throws PlatformNotSupportedExceptions in most methods.

Instead, it needs to pick up runtimes\unix\lib\netcoreapp2.1\System.Data.SqlClient.dll .

I have verified that a simple console app published for must gets the right one. So this has to do with some customization of package restore that we got in the build. Do you happen to know where that may happen?

A simple way to fix this is to switch back to copying System.Data.SqlClient.dll to testhost. I will do that.

jkotas · 2020-02-06T11:47:52Z

#31850

Dotnet-GitSync-Bot added the area-System.Data.SqlClient label Jan 28, 2020

jkotas commented Jan 28, 2020

View reviewed changes

src/libraries/shims/ApiCompatBaseline.netcoreapp.netstandard.txt Outdated Show resolved Hide resolved

jkotas requested review from ericstj, danmoseley, cheenamalhotra and David-Engel January 28, 2020 08:14

stephentoub reviewed Jan 28, 2020

View reviewed changes

src/coreclr/tests/scripts/run-corefx-tests.bat Show resolved Hide resolved

ericstj reviewed Jan 28, 2020

View reviewed changes

src/libraries/restore/binplacePackages/binplacePackages.depproj Outdated Show resolved Hide resolved

jkotas mentioned this pull request Feb 4, 2020

Fix issues with SqlCommandSet not working with Byte Array parameters #1462

Closed

ericstj reviewed Feb 4, 2020

View reviewed changes

src/libraries/shims/manual/System.Data.csproj Show resolved Hide resolved

jaredpar mentioned this pull request Feb 5, 2020

System.Security.Cryptography.OpenSsl.Tests failing on CI runs #2176

Closed

jkotas mentioned this pull request Feb 5, 2020

[mono] Wrong System.Data.SqlClient.dll loaded #31799

Closed

ViktorHofer reviewed Feb 5, 2020

View reviewed changes

src/libraries/shims/manual/System.Data.csproj Show resolved Hide resolved

ViktorHofer reviewed Feb 5, 2020

View reviewed changes

....Runtime.Serialization.Formatters/tests/System.Runtime.Serialization.Formatters.Tests.csproj Outdated Show resolved Hide resolved

ViktorHofer approved these changes Feb 5, 2020

View reviewed changes

jkotas added 2 commits February 5, 2020 08:07

Delete System.Data.SqlClient package

b201592

Fixes https://github.com/dotnet/corefx/issues/40846

Delete unnecessary SqlClient references

8724e08

jkotas added 5 commits February 5, 2020 08:07

Unify System.Data.SqlClient package reference

0cc1f57

Add System.Data facade test

1caf492

Disable tests on Mono

b225209

Define System.Data.SqlClient version in Versions.props

d38f725

safern approved these changes Feb 5, 2020

View reviewed changes

jkotas merged commit 722cf30 into dotnet:master Feb 5, 2020

This was referenced Feb 6, 2020

Fix System.Data.SqlClient rolling tests on Alpine #31850

Merged

Future of System.Data.SqlClient for 5.0 #30772

Closed

jkotas deleted the sqlclient branch February 7, 2020 20:02

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete System.Data.SqlClient package #2275

Delete System.Data.SqlClient package #2275

jkotas commented Jan 28, 2020 •

edited

Loading

stephentoub commented Jan 28, 2020 •

edited

Loading

jkotas commented Jan 28, 2020 •

edited

Loading

ericstj commented Jan 28, 2020 •

edited

Loading

Wraith2 commented Jan 28, 2020

ericstj commented Jan 28, 2020

jkotas commented Jan 28, 2020

Wraith2 commented Jan 30, 2020

ViktorHofer commented Feb 3, 2020

Wraith2 commented Feb 3, 2020

jkotas commented Feb 3, 2020

ericstj commented Feb 4, 2020

safern commented Feb 5, 2020

jkotas commented Feb 5, 2020

ViktorHofer left a comment

jkotas commented Feb 5, 2020

ViktorHofer commented Feb 5, 2020 •

edited

Loading

jkotas commented Feb 5, 2020

safern commented Feb 6, 2020

jkotas commented Feb 6, 2020

jkotas commented Feb 6, 2020

Delete System.Data.SqlClient package #2275

Delete System.Data.SqlClient package #2275

Conversation

jkotas commented Jan 28, 2020 • edited Loading

stephentoub commented Jan 28, 2020 • edited Loading

jkotas commented Jan 28, 2020 • edited Loading

ericstj commented Jan 28, 2020 • edited Loading

Wraith2 commented Jan 28, 2020

ericstj commented Jan 28, 2020

jkotas commented Jan 28, 2020

Wraith2 commented Jan 30, 2020

ViktorHofer commented Feb 3, 2020

Wraith2 commented Feb 3, 2020

jkotas commented Feb 3, 2020

ericstj commented Feb 4, 2020

safern commented Feb 5, 2020

jkotas commented Feb 5, 2020

ViktorHofer left a comment

Choose a reason for hiding this comment

jkotas commented Feb 5, 2020

ViktorHofer commented Feb 5, 2020 • edited Loading

jkotas commented Feb 5, 2020

safern commented Feb 6, 2020

jkotas commented Feb 6, 2020

jkotas commented Feb 6, 2020

jkotas commented Jan 28, 2020 •

edited

Loading

stephentoub commented Jan 28, 2020 •

edited

Loading

jkotas commented Jan 28, 2020 •

edited

Loading

ericstj commented Jan 28, 2020 •

edited

Loading

ViktorHofer commented Feb 5, 2020 •

edited

Loading