jonwagner / Insight.Database

Fast, lightweight .NET micro-ORM
Other
861 stars 145 forks source link

Why does Insight depend on System.Data packages for .NET Framework? #363

Closed sharpjs closed 6 years ago

sharpjs commented 6 years ago

Type

Question

Current Behavior

Installing Insight.Database on a .NET Framework 4.7.1 project using a traditional packages.config file causes System.Data.Common and System.Data.SqlClient packages to be installed as dependencies.

Expected behavior

Insight.Database references the System.Data assemblies shipped with the .NET Framework.

Steps to Reproduce

Install Insight in a .NET Framework 4.7.1 project using a packages.config file.

Any other comment

This is confusing. As I understand it, those packages are for .NET Core/Standard. I don't know why those packages exist for .NET Framework, since the framework ships those assemblies already. Which ones are the latest version?

Ultimately, I need to know how I should reconcile Insight's dependencies with other packages that depend on the Framework's System.Data assemblies. Which System.Data.Foo wins? Do I need binding redirects?

jonwagner commented 6 years ago

Insight requires Common and SqlClient in order to connect to the database. For each target platform, we specify the minimum version that is required to support Insight's functionality. The .NET platform is supposed to resolve this automatically.

Is there a specific problem that you are trying to address?

sharpjs commented 6 years ago

My apologies if the question seems a tad ridiculous. This surely boils down to my own lack of understanding. I just haven't been able to find the answer.

I know why the System.Data assemblies are required. In fact, I am intimately comfortable with the ADO.NET API. The question isn't why Insight needs those assemblies; it's why Insight wants the NuGet package specifically, as opposed to using the versions shipped with the Framework.

The problem I'm trying to solve is to update Insight from 5.2.8 to 6.2.0, while ensuring that I don't break other packages that depend on the System.Data assemblies. The automatic edits that VS/NuGet make during package install are sometimes inappropriate, requiring manual correction. To make those corrections, I need to understand the versioning of all nodes in my dependency graph and what changes occur between those versions. The "hit 'update all' and hope" strategy does not work for my product.

With Insight 5.2.8, my csproj files contain:

<Reference Include="System.Data" />

When my product uses types like DbConnection and SqlConnection, it gets the implementation that shipped with .NET Framework 4.7.1.

Upgrading to Insight 6.2.0, my csproj files contain:

<Reference Include="System.Data" />
<Reference Include="System.Data.Common">
  <HintPath>..\packages\System.Data.Common.4.1.0\lib\net451\System.Data.Common.dll</HintPath>
  <Private>True</Private>
</Reference>
<Reference Include="System.Data.SqlClient, Version=4.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">
  <HintPath>..\packages\System.Data.SqlClient.4.1.0\lib\net46\System.Data.SqlClient.dll</HintPath>
</Reference>

My product will now ship specific System.Data assemblies, and I don't understand how this will affect other things that depend on those assemblies. Specifically:

So, my question really isn't about Insight itself; it's about these System.Data NuGet packages and how they relate to the same assemblies shipped with the .NET Framework. I asked here in hopes that you might have some guidance, since you changed Insight to reference those packages.

jonwagner commented 6 years ago

Those are good questions! I don't have all of the answers, so let me do some digging.

Generally, newer assemblies should be backward-compatible, but that's not always the case. We also want to make sure that assembly dependencies don't leak to other projects unintentionally.

andymac4182 commented 6 years ago

I think this will be related to this reference. https://github.com/jonwagner/Insight.Database/blob/f0cea5332e7443992f2c7ebfa3bf638e4ae1b426/Insight.Database.Providers.Default/Insight.Database.Providers.Default.csproj#L17 It probably would need to be wrapped similar to https://github.com/jonwagner/Insight.Database/blob/f0cea5332e7443992f2c7ebfa3bf638e4ae1b426/Insight.Database.Core/Insight.Database.Core.csproj#L20 so that it uses the reference to the built in version for full framework and the nuget reference for netcore. Personally I think this is ok since more and more packages are dropping a default reference to full framework and just targeting netstandard which using the nuget is the only way to use these references.

sharpjs commented 6 years ago

Here's what I see in ILSpy looking at a DLL from the NuGet package. It just forwards all types to the underlying .NET Framework DLL.

screen shot 2018-04-20 at 11 50 04 am

So, in one sense, these seem harmless in that the various dependents are going to get the .NET Framework 4.7.1 System.Data.dll implementations regardless. On the other hand, it is confusing and easily avoided. Similar to what @andymac4182 wrote, I've done it like this before:

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.5'">
  <PackageReference Include="System.Data.SqlClient" Version="$(SqlClientVersion)" Condition="$(SqlClientVersion) != ''" /> 
</ItemGroup>

I don't know if it matters, but I've always seen single quotes around the left-hand side in the Condition attributes.

Edit: Might also be resolved by modifying here: https://github.com/jonwagner/Insight.Database/blob/f0cea5332e7443992f2c7ebfa3bf638e4ae1b426/SharedConfiguration.csproj#L42 I don't know if that addresses System.Data.Common though.

jonwagner commented 6 years ago

Should be easy to fix then

On Apr 20, 2018, at 5:59 PM, Jeff Sharp notifications@github.com<mailto:notifications@github.com> wrote:

Here's what I see in ILSpy looking at a DLL from the NuGet package. It just forwards all types to the underlying .NET Framework DLL.

[screen shot 2018-04-20 at 11 50 04 am]https://user-images.githubusercontent.com/708461/39075420-6183040c-44aa-11e8-9364-721a62010e49.png

So, in one sense, these seem harmless in that the various dependents are going to get the .NET Framework 4.7.1 System.Data.dll implementations regardless. On the other hand, it is confusing and easily avoided. Similar to what @andymac4182https://github.com/andymac4182 wrote, I've done it like this before:

I don't know if it matters, but I've always seen quotes around the left-hand side in the Condition attributes.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/jonwagner/Insight.Database/issues/363#issuecomment-383233627, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABk3dKaLnA_rDBKia9ufUZD-r-r06nOIks5tqlo4gaJpZM4TciNx.

jonwagner commented 6 years ago

I removed the explicit dependencies from .net framework builds in v6.2.1. Let me know if you still have an issue.

sharpjs commented 6 years ago

Thanks, @jonwagner. Just tried it, and the Insight.Database.Configuration project still depends on one, specifically in the net451 build.

screen shot 2018-05-13 at 4 34 29 pm
jonwagner commented 6 years ago

v6.2.2. has the config project fixed as well.

sharpjs commented 6 years ago

Hey @jonwagner , 6.2.2 still has the dependency. Here's what I see in NuGet.org:

screen shot 2018-05-25 at 3 34 11 pm

Jaxelr commented 6 years ago

Could you check your cache? It looks correctly at least on my end.

Heres the nuget page:

image

And heres the Nuget window on VS 2017:

image

jonwagner commented 6 years ago

Sorry that was a merge error on my part :(

jonwagner commented 6 years ago

Alrighty...v6.2.3 no longer has a library dependency when targeting .NET Framework.

I checked Core, Configuration, Providers.Default, and Json packages. Let me know if you find any more.

sharpjs commented 6 years ago

Thanks! Looks good here.